[GitHub] tweise commented on issue #7249: [FLINK-11048] Ability to programmatically execute streaming pipeline with savepoint restore
tweise commented on issue #7249: [FLINK-11048] Ability to programmatically execute streaming pipeline with savepoint restore URL: https://github.com/apache/flink/pull/7249#issuecomment-449118180 @mxm Let's leave it as is. The main point was addition of the static method, which effectively decouples `StreamExecutionEnvironment` from where the job is executed. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] tweise commented on issue #7249: [FLINK-11048] Ability to programmatically execute streaming pipeline with savepoint restore
tweise commented on issue #7249: [FLINK-11048] Ability to programmatically execute streaming pipeline with savepoint restore URL: https://github.com/apache/flink/pull/7249#issuecomment-449083100 @mxm the contract requires us to call this method (we could also not modify the existing execute method to bypass it). Hence my earlier revision stored the savepoint settings in a member variable, which IMO would have been a reasonable compromise: https://github.com/apache/flink/pull/7249/commits/c10f8dfc390de2b24b5037daa8cef5749389106c This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] tweise commented on issue #7249: [FLINK-11048] Ability to programmatically execute streaming pipeline with savepoint restore
tweise commented on issue #7249: [FLINK-11048] Ability to programmatically execute streaming pipeline with savepoint restore URL: https://github.com/apache/flink/pull/7249#issuecomment-449078240 > Just to sum up my review. > > * Personally would prefer method `execute(String jobName, SavepointSettings savepointSettings`, but I am ok with the ctor as well > * Before merging this PR please annotate newly introduced methods/ctor with `@Internal` or at least `@PublicEvolving` > > After that I am +1 @dawidwys thanks for the review. I marked the new method `public static JobExecutionResult executeRemotely` as evolving. I would also prefer the execute method with the savepoint restore settings (the previous version of the PR had that actually). Unfortunately we had to discard that (in favor of the constructor) because it would require the savepoint restore parameter to be passed through a member variable to still be able to call `protected JobExecutionResult executeRemotely(StreamGraph streamGraph, List jarFiles)`, which we have to for backward compatibility.. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] tweise commented on issue #7249: [FLINK-11048] Ability to programmatically execute streaming pipeline with savepoint restore
tweise commented on issue #7249: [FLINK-11048] Ability to programmatically execute streaming pipeline with savepoint restore URL: https://github.com/apache/flink/pull/7249#issuecomment-447403037 @mxm I think the constructor proliferation suffers from its own issues: besides repetition you need to decide how to prioritize parameters that naturally have no priority. And again anything with a long parameter list is not only hard to read but also toxic for backward compatible code evolution. Ideally we would just have one parameter to execute, which could be of type `ExecutionParameters` and hold the job name as well as the savepoint info. Anything else that might be needed in the future can be added without breaking the interface contract. But that isn't easy to accomplish due to how the code has been cast. The difficulty comes from the protected executeRemotely method that we cannot change. How about passing the savepoint parameter or before mentioned new parameters holder through a thread local instead of the instance variable? The difference how we pass it internally is cosmetic and not important to the user. The question of savepoint vs. generalized execution parameters seems more interesting. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
