Flink: Tests alignment for the Flink Sink v2-based implemenation (IcebergSink)

[arkadius]

The scope of this PR is to provide the same level of test use case coverage for the new implementation of Flink's sink (IcebergSink) based on Flink's v2 sinks API as it was done for the previous implementation (FlinkSink). The next step after this change will be to add a configuration option allowing to choose which implementation of the sink to use - this will be done in a separate PR.

This PR is the follow-up of discussion in #10179

[pvary]

@rodmeneses: Please review

[rodmeneses]

The scope of this PR is to provide unit tests for the new implementation of Flink's sink (IcebergSink) based on Flink's v2 sinks API. This PR is the follow-up of discussion in #10179

Hi @arkadius . I would update the PR description to something like: The scope of this PR is to add unit tests for the IcebergSink implementation in all the modules where FlinkSink is referenced, like: <....$$$ please list what components you are referring to. ie: DynamicTables [however, keep in mind that I think there are no existing unit tests for dynamicTables, even with the FlinkSink implementation.$$$

My reasoning here is that the current description kind of implies that IcebergSink doesn't have unit tests, which it does.

[arkadius]

The scope of this PR is to provide unit tests for the new implementation of Flink's sink (IcebergSink) based on Flink's v2 sinks API. This PR is the follow-up of discussion in #10179

Hi @arkadius . I would update the PR description to something like: The scope of this PR is to add unit tests for the IcebergSink implementation in all the modules where FlinkSink is referenced, like: <....$$$ please list what components you are referring to. ie: DynamicTables [however, keep in mind that I think there are no existing unit tests for dynamicTables, even with the FlinkSink implementation.$$$

My reasoning here is that the current description kind of implies that IcebergSink doesn't have unit tests, which it does.

You are right, The summary is confusing. I'll propose the change of it when we close the discussion about the scope of this change.

however, keep in mind that I think there are no existing unit tests for dynamicTables, even with the FlinkSink implementation.$$$

I'm a little bit confused about that. By dynamic tables, I thought about this concept https://nightlies.apache.org/flink/flink-docs-master/docs/dev/table/concepts/dynamic_tables/ . From this perspective, everywhere where we use Table API or Flink SQL, and the test code touches FlinkDynamicTableFactory (via SPI or directly), we test Iceberg's dynamic tables implementation. Did you think about something else?

[arkadius]

@rodmeneses , @pvary I've opened the new PR #11249 with non-controversial changes that I've discovered during this work. It contains mostly goodies that were added in IcebergSink tests and haven't been migrated to FlinkSink tests. Can you take a look at them?

[rodmeneses]

Hi @arkadius I have started working in backporting the RANGE distribution to the IcebergSink. The unit tests in my code will benefit from the new marker interface you are introducing in this PR, so I'd like to merge this one so that I can rebase properly. I see this PR is still on "draft". Is anything pending?

@pvary does this PR look good to you, or you have anything pending that would like to be done? The PR is looking good to me.

Also, please @pvary and @stevenzwu please help starting the CI pipelines on this PR.

[arkadius]

Hi @arkadius I have started working in backporting the RANGE distribution to the IcebergSink. The unit tests in my code will benefit from the new marker interface you are introducing in this PR, so I'd like to merge this one so that I can rebase properly. I see this PR is still on "draft". Is anything pending?

@pvary does this PR look good to you, or you have anything pending that would like to be done? The PR is looking good to me.

Also, please @pvary and @stevenzwu please help starting the CI pipelines on this PR.

@rodmeneses I think that it would be better if you copied the parts of the code from this PR into your PR. I can rebase into your changes after you merge them.

This PR isn't finished yet. I have a plan how the final shape of unit test could look like to make them easier to maintain and to fill some gaps in IcebergSink's tests but before I show it, I would like to merge this PR: #11249 (and maybe this: #11244). Thanks to that, it will be more clear what is the scope of this change.

[rodmeneses]

Hi @arkadius I have started working in backporting the RANGE distribution to the IcebergSink. The unit tests in my code will benefit from the new marker interface you are introducing in this PR, so I'd like to merge this one so that I can rebase properly. I see this PR is still on "draft". Is anything pending? @pvary does this PR look good to you, or you have anything pending that would like to be done? The PR is looking good to me. Also, please @pvary and @stevenzwu please help starting the CI pipelines on this PR.

@rodmeneses I think that it would be better if you copied the parts of the code from this PR into your PR. I can rebase into your changes after you merge them.

This PR isn't finished yet. I have a plan how the final shape of unit test could look like to make them easier to maintain and to fill some gaps in IcebergSink's tests but before I show it, I would like to merge this PR: #11249 (and maybe this: #11244). Thanks to that, it will be more clear what is the scope of this change.

It could make sense, to open a PR with only Builder Marker Interface. I can raise that PR, if you're ok with it @arkadius. But before jumping into that, I'd like to hear about this from @pvary and/or @stevenzwu. What do you think?

[arkadius]

Hi @arkadius I have started working in backporting the RANGE distribution to the IcebergSink. The unit tests in my code will benefit from the new marker interface you are introducing in this PR, so I'd like to merge this one so that I can rebase properly. I see this PR is still on "draft". Is anything pending? @pvary does this PR look good to you, or you have anything pending that would like to be done? The PR is looking good to me. Also, please @pvary and @stevenzwu please help starting the CI pipelines on this PR.

@rodmeneses I think that it would be better if you copied the parts of the code from this PR into your PR. I can rebase into your changes after you merge them. This PR isn't finished yet. I have a plan how the final shape of unit test could look like to make them easier to maintain and to fill some gaps in IcebergSink's tests but before I show it, I would like to merge this PR: #11249 (and maybe this: #11244). Thanks to that, it will be more clear what is the scope of this change.

It could make sense, to open a PR with only Builder Marker Interface. I can raise that PR, if you're ok with it @arkadius. But before jumping into that, I'd like to hear about this from @pvary and/or @stevenzwu. What do you think?

It's fine for me. I raised it here #11305. We can move this discussion there.

[pvary]

Merged #11305

[github-actions[bot]]

This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the [email protected] list. Thank you for your contributions.

[rodmeneses]

Hi @arkadius is this still needed/relevant ? please advise, as it will be closed due to inactivity soon cc @pvary

[arkadius]

Hi @rodmeneses. No, this change is not relevant. I'll close it.