-
Notifications
You must be signed in to change notification settings - Fork 501
[FLINK-37885] Upgrade to JOSDK 5.1 #982
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
Conversation
Signed-off-by: Attila Mészáros <[email protected]>
Signed-off-by: Attila Mészáros <[email protected]>
Signed-off-by: Attila Mészáros <[email protected]>
Signed-off-by: Attila Mészáros <[email protected]>
Signed-off-by: Attila Mészáros <[email protected]>
Signed-off-by: Attila Mészáros <[email protected]>
Signed-off-by: Attila Mészáros <[email protected]>
Signed-off-by: Attila Mészáros <[email protected]>
Signed-off-by: Attila Mészáros <[email protected]>
gyfora
left a comment
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.
The Java 11 -> 17 upgrade requirement for the operator must be discussed on the Flink Mailing list before this is merged
Signed-off-by: Attila Mészáros <[email protected]>
Signed-off-by: Attila Mészáros <[email protected]>
| InformerConfiguration<FlinkSessionJob> configuration = | ||
| InformerConfiguration.from(FlinkSessionJob.class, context) | ||
| var configuration = | ||
| InformerEventSourceConfiguration.from(FlinkSessionJob.class, FlinkSessionJob.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.
should this be InformerEventSourceConfiguration.from(FlinkSessionJob.class, FlinkDeployment.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.
fixed, good catch, thank you!
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.
I wonder how this worked in our e2es or if we are missing some key test coverage :D
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.
Actually not, it is just used for the default secondayr to primary mapper. So actually the API might be on JOSDK side. So this is not a mandatory field.
gyfora
left a comment
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.
Overall looks good, I had a minor comment/question
Signed-off-by: Attila Mészáros <[email protected]>
What is the purpose of the change
To update the to the most recent version of JOSDK (5.1)
Brief change log
UpdateControl.updateResourceis replaced byUpdateControl.patchResource.Verifying this change
This change is a trivial rework / code cleanup without any test coverage.
Does this pull request potentially affect one of the following parts:
CustomResourceDescriptors: Theoretically yesDocumentation