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

Migrate HadoopCatalog related tests in Flink #10358

Merged
merged 1 commit into from
Jul 1, 2024

Conversation

tomtongue
Copy link
Contributor

@tomtongue tomtongue commented May 20, 2024

Migrate the following classes in iceberg-flink to JUnit 5 for #9087

FlinkTestBase inherited classes (currently only including v1.19)

  • TestFlinkIcebergSink
  • TestFlinkIcebergSinkBranch
  • TestFlinkIcebergSinkV2
  • TestFlinkIcebergSinkV2Branch
  • TestSqlBase
    • TestFlinkSourceSql
    • TestIcebergSourceSql
  • (HadoopTableResource related test) TestColumnStatsWatermarkExtractor and ReaderUtil

And newly created file for HadoopTableResource alternative:

  • HadoopTableExtension

@github-actions github-actions bot added the flink label May 20, 2024
@tomtongue tomtongue changed the title [WIP] Migrate HadoopCatalog related tests [WIP] Migrate HadoopCatalog related tests in Flink May 20, 2024
@tomtongue
Copy link
Contributor Author

Other HadoopTableResource related files will be migrated in another PR.

@tomtongue tomtongue force-pushed the mig-junit5-flink-hadoopcatalog branch from 8624b13 to 777c3a6 Compare May 21, 2024 07:51
@tomtongue tomtongue changed the title [WIP] Migrate HadoopCatalog related tests in Flink Migrate HadoopCatalog related tests in Flink May 22, 2024
@tomtongue
Copy link
Contributor Author

tomtongue commented May 22, 2024

@nastra Sorry for the delay. Could you review this PR when you have time? This PR includes a bit file changes. so if it's fine, I will add backports to other versions to this PR.

@@ -50,4 +51,17 @@ public static MiniClusterExtension createWithClassloaderCheckDisabled() {
.setConfiguration(DISABLE_CLASSLOADER_CHECK_CONFIG)
.build());
}

public static MiniClusterExtension createWithClassloaderCheckDisabled(
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is necessary (I don't see it being used anywhere)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! Let me remove this in the next commit.

@@ -122,4 +123,24 @@ public static CombinedScanTask createCombinedScanTask(

return new BaseCombinedScanTask(fileTasks);
}

public static CombinedScanTask createCombinedScanTask(
Copy link
Contributor

Choose a reason for hiding this comment

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

can you elaborate why this change is needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The other createCombinedScanTask (described above in this class) is still used in other test classes which include JUnit4. This method still takes the TemporaryFolder argument. So, for this commit, I choose the method overloading. Once the other classes are migrated into JUnit5, I will remove the old createCombinedScanTask (which takes the TemporaryFolder argument). I will add a comment line for this.

Copy link
Contributor

@nastra nastra left a comment

Choose a reason for hiding this comment

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

changes LGTM other than the 2 I commented on

@tomtongue
Copy link
Contributor Author

Thanks for the review! and, sorry for much delay. The changes have conflics, so let me fix them and will commit again.

@tomtongue tomtongue force-pushed the mig-junit5-flink-hadoopcatalog branch from 5df014a to e801ce5 Compare July 1, 2024 12:01
@tomtongue
Copy link
Contributor Author

tomtongue commented Jul 1, 2024

@nastra Hi Eduard! When you have time, could you review the latest change again?

Copy link
Contributor

@nastra nastra left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for working on this @tomtongue. Can you also please backport this to earlier Flink versions?

@nastra nastra merged commit a975a95 into apache:main Jul 1, 2024
19 checks passed
@tomtongue
Copy link
Contributor Author

Thank you! Yes, I will submit the backport PR for this.

@tomtongue tomtongue deleted the mig-junit5-flink-hadoopcatalog branch July 2, 2024 01:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants