-
Notifications
You must be signed in to change notification settings - Fork 141
Client refactor #1192
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Client refactor #1192
Conversation
* added default argument loader * added Type in <> operator * removed default loader --------- Co-authored-by: Nitish <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
listeners are missing? thats the main thing we want to refactor. please also check other comments.
| this.factoryClass = factoryClass; | ||
| } | ||
|
|
||
| @SuppressWarnings("unchecked") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
factory and izingg should not be part of the same class
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Segregated factory and Zingg
| return new ArgumentsAssembler(); | ||
| } | ||
| protected ZinggFactoryProvider<S, D, R, C> getZinggFactoryProvider() { | ||
| return new ZinggFactoryProvider<>("zingg.spark.core.executor.SparkZFactory"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this si spark related, should not be here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed. made as abstract method
| .master("local[*]") | ||
| .config("spark.sql.shuffle.partitions", "8") | ||
| .getOrCreate(); | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing all the checkpoint relaed work we did
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated here. all the changes are pulled
| SparkSession spark = SparkSession.builder() | ||
| .appName("Zingg") | ||
| .master("local[*]") | ||
| .config("spark.sql.shuffle.partitions", "8") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this was never hardcoded, where is this comign from?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed hardcoding
listeners are integrated |
| } | ||
| } | ||
|
|
||
| public static String getProductName(){ return "Zingg AI"; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can these be static, as we will override in enterprise?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
| public final Log LOG = LogFactory.getLog(ArgumentsAssembler.class); | ||
|
|
||
| public IZArgs assemble(IZArgs args, ClientOptions options) { | ||
| int jobId = (int) (System.currentTimeMillis()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a long. thats why original code did int jobId = new Long(System.currentTimeMillis()).intValue();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated it
* reformatted PipeUtil * break reader and writer as two separate components * changes * changed variable scope * added Helper class for initializing reader * updated WriterStrategyFactory * updated PipeUtilBase * renamed interfaces names and added Helper to for initializing writer * removed loader with string * added default reader and writer strategy * removed inMemory read strategy * changed location to path property * removed in-memory pipes * made variable protected * updated strategy reader * added back compatibility for location * removed helper from reader and writer * added pipe to writer strategy * updated snowflake format * reverted back snowflake format * added constant location and path --------- Co-authored-by: Nitish <[email protected]>
No description provided.