[FLINK-22915][FLIP-173] Updates the static load(...) method of Stage subclasses to take StreamExecutionEnvironment as parameter #33
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@gaoyunhaii Could you review this PR? |
lindong28
changed the title
lindong28
force-pushed
the
FLINK-24354-followup
branch
2 times, most recently
from
82eee62
to
8b05243
Compare
lindong28
changed the title
lindong28
changed the title
gaoyunhaii
reviewed
Nov 16, 2021
| @@ -39,6 +39,7 @@ | |||
| void save(String path) throws IOException; | |||
|
|
|||
| // NOTE: every Stage subclass should implement a static method with signature "static T | |||
| // load(String path)", where T refers to the concrete subclass. This static method should | |||
| // instantiate a new stage instance based on the data from the given path. | |||
| // load(StreamExecutionEnvironment env, String path)", where T refers to the concrete | |||
There was a problem hiding this comment.
We might move this part of comment to the class comments? Then it would also appear in the java doc.
Also change static T load(StreamExecutionEnvironment env, String path) -> {@code static T load(StreamExecutionEnvironment env, String path)} and T -> {@code T}
There was a problem hiding this comment.
Good point. The PR has been updated as suggested.
…subclasses to take StreamExecutionEnvironment as parameter
…e Model instance itself
lindong28
force-pushed
the
FLINK-24354-followup
branch
from
8b05243
to
e2a1a7f
Compare
gaoyunhaii
approved these changes
Nov 16, 2021
There was a problem hiding this comment.
Very thanks @lindong28 for the update! LGTM and +1 to merge
|
Thanks for the prompt review @gaoyunhaii! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What is the purpose of the change
This PR updates the static load(...) method of Stage subclasses to take StreamExecutionEnvironment as parameter. This is because the static load method will need to create DataStream or Table instances to read model data.
This PR also updates Model::setModelData(...) to return the Model instance itself. This could improve usability by allowing users to do e.g.
return new KMeansModel().setModelData(...).Brief change log
This PR mades the following changes:
Stagesubclasses to take StreamExecutionEnvironment as parameter.ReadWriteUtilsto take StreamExecutionEnvironment as parameter.Verifying this change
The changes are validated by the existing unit tests.
Does this pull request potentially affect one of the following parts:
@Public(Evolving): (no)Documentation