Skip to content
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

[issue-730] Upgrade to flink 1.18.1 #731

Merged
merged 6 commits into from
Feb 6, 2024
Merged

Conversation

GaMalhot
Copy link
Contributor

@GaMalhot GaMalhot commented Jan 31, 2024

Change log description

  1. Upgrade of Flink version to 1.18.1 on master branch
  2. Changes w.r.t FLINK_32376 for Sink Init Context
  3. Changes w.r.t FLINK-31972 for fixing the ambiguous usage of assertThat

Purpose of the change
Fix: #730

What the code does
Upgrade flinkVersion to 1.18.1 in gradle.properties

How to verify it
./gradlew clean build should pass

@GaMalhot GaMalhot marked this pull request as ready for review January 31, 2024 10:19
@GaMalhot GaMalhot marked this pull request as draft January 31, 2024 10:19
@GaMalhot GaMalhot marked this pull request as ready for review February 1, 2024 07:24
@RaulGracia RaulGracia requested a review from crazyzhou February 1, 2024 09:10
Copy link

codecov bot commented Feb 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (faca104) 81.12% compared to head (8b3f5ef) 81.23%.

Additional details and impacted files
@@             Coverage Diff              @@
##             master     #731      +/-   ##
============================================
+ Coverage     81.12%   81.23%   +0.11%     
- Complexity      573      574       +1     
============================================
  Files            62       62              
  Lines          2723     2723              
  Branches        232      232              
============================================
+ Hits           2209     2212       +3     
+ Misses          326      324       -2     
+ Partials        188      187       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@crazyzhou crazyzhou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add some reference Flink tickets of your code changes with your PR description, so that we can keep track, like FLINK-32376 and FLINK-31972

@@ -135,7 +135,7 @@ public void testCreateCatalogFromFactory() {
assertThat(actualCatalog instanceof PravegaCatalog).isTrue();
assertThat(((PravegaCatalog) actualCatalog).getName()).isEqualTo(CATALOG.getName());
assertThat(((PravegaCatalog) actualCatalog).getDefaultDatabase()).isEqualTo(CATALOG.getDefaultDatabase());
assertThat(Whitebox.getInternalState(actualCatalog, "properties"))
org.assertj.core.api.Assertions.assertThat((Map) Whitebox.getInternalState(actualCatalog, "properties"))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can use assertThat directly

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Brian, the issue has been handled.

@@ -146,6 +148,21 @@ private static class SinkInitContext implements Sink.InitContext {
SinkInitContext(
SinkWriterMetricGroup metricGroup) {
this.metricGroup = metricGroup;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indentation not correct here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Brian, The indentation has been fixed.

@GaMalhot
Copy link
Contributor Author

GaMalhot commented Feb 4, 2024

Please add some reference Flink tickets of your code changes with your PR description, so that we can keep track, like FLINK-32376 and FLINK-31972

Hi @crazyzhou , I have made changes and added the references in the description.

Copy link

@yaol7 yaol7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@fyang86 fyang86 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link

@ShwethaSNayak ShwethaSNayak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@RaulGracia RaulGracia merged commit bcc4e07 into pravega:master Feb 6, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrade to flink 1.18.1
6 participants