Skip to content

Flink: Fix flaky test TestFlinkTableSink - #2989

Merged
openinx merged 4 commits into
apache:masterfrom
szehon-ho:fix_flaky2
Sep 4, 2021
Merged

Flink: Fix flaky test TestFlinkTableSink#2989
openinx merged 4 commits into
apache:masterfrom
szehon-ho:fix_flaky2

Conversation

@szehon-ho

@szehon-hoszehon-ho commented Aug 17, 2021•
edited
Loading

Copy link
Copy Markdown
Member
  • In my observation, new files may be added if pass checkpoint interval (set to 400)

@szehon-hoszehon-ho mentioned this pull request Aug 17, 2021
Merged
@szehon-ho

szehon-ho commented Aug 17, 2021

Copy link
Copy Markdown
MemberAuthor

@stevenzwu@openinx can you guys help check my understanding? Thanks

@stevenzwu

stevenzwu commented Aug 18, 2021•
edited
Loading

Copy link
Copy Markdown
Contributor

@szehon-ho I am not sure why this failed. maybe it is what you said. records were split across two checkpoint/commit cycles. However, this change will defeat the purpose of this unit test. with hash distribution, this unit test is to make sure all records for the same partition key is distributed to a single writer task and hence should generate one file per partition (per commit cycle).

Here is probably what we should do to fix the flakiness. we should assert that there is only one file per partition for each commit/snapshot.

@szehon-ho

szehon-ho commented Aug 18, 2021

Copy link
Copy Markdown
MemberAuthor

Yea this makes sense, let me look into it.

@szehon-ho

szehon-ho commented Aug 27, 2021

Copy link
Copy Markdown
MemberAuthor

@stevenzwu thanks for the review. I put back the one-file-per-partition, per commit/checkpoint. Also I added some test cleanup as well in the failure case.

stevenzwu
stevenzwu reviewed Aug 28, 2021
View reviewed changes
Comment threadflink/src/test/java/org/apache/iceberg/flink/SimpleDataUtil.java Outdated
stevenzwu
stevenzwu reviewed Aug 28, 2021
View reviewed changes
Comment threadflink/src/test/java/org/apache/iceberg/flink/SimpleDataUtil.java Outdated
stevenzwu
stevenzwu reviewed Aug 28, 2021
View reviewed changes
Comment threadflink/src/test/java/org/apache/iceberg/flink/SimpleDataUtil.java Outdated
stevenzwu
stevenzwu reviewed Aug 28, 2021
View reviewed changes
Comment threadflink/src/test/java/org/apache/iceberg/flink/SimpleDataUtil.java
List<DataFile> dataFiles, PartitionSpec partitionSpec, Map<String, Object> partitionValues) {
Types.StructType spec = partitionSpec.partitionType();
Record partitionRecord = GenericRecord.create(spec).copy(partitionValues);
StructLikeWrapper expected = StructLikeWrapper

@stevenzwustevenzwuAug 28, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

PartitionData implements the equals method. we can construct PartitionData using this API from DataFiles class. not sure if it is better. but at least it is more specific.

 public static PartitionData copy(PartitionSpec spec, StructLike partition) {
return copyPartitionData(spec, partition, null);
}

@szehon-hoszehon-hoSep 2, 2021

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

That's a good idea, but when I tried I found PartitionData is package protected so can't access it here unfortunately.

stevenzwu
stevenzwu reviewed Aug 28, 2021
View reviewed changes
Comment threadflink/src/test/java/org/apache/iceberg/flink/TestFlinkTableSink.java Outdated
@stevenzwu

stevenzwu commented Aug 28, 2021

Copy link
Copy Markdown
Contributor

@szehon-ho thx a lot for fixing the flaky tests. overall, it looks good to me. Left a few minor comments.

@szehon-ho

szehon-ho commented Sep 2, 2021

Copy link
Copy Markdown
MemberAuthor

@stevenzwu thanks again for taking a look, I updated based on the feedback

stevenzwu
stevenzwu approved these changes Sep 2, 2021
View reviewed changes

@stevenzwustevenzwu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM. @openinx can you take a look too and merge it when it is good from your side.

openinx
openinx reviewed Sep 3, 2021
View reviewed changes
Comment threadflink/src/test/java/org/apache/iceberg/flink/SimpleDataUtil.java Outdated
Comment on lines +269 to +271
public static Map<Long, List<DataFile>> snapshotToDataFiles(
Table table)
throws IOException {

@openinxopeninxSep 3, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nit: The method format looks quite strange for me, could we format it looks like the following?

 public static Map<Long, List<DataFile>> snapshotToDataFiles(Table table) throws IOException {

@szehon-hoszehon-hoSep 3, 2021

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Done

openinx
openinx reviewed Sep 3, 2021
View reviewed changes
Comment threadflink/src/test/java/org/apache/iceberg/flink/SimpleDataUtil.java Outdated
List<ManifestFile> manifestFiles = table.currentSnapshot().dataManifests();
for (ManifestFile manifestFile : manifestFiles) {
try (ManifestReader<DataFile> reader = ManifestFiles.read(manifestFile, table.io())) {
List<DataFile> dataFiles = IteratorUtils.toList(reader.iterator());

@openinxopeninxSep 3, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I see a warning in IDE here, using the Lists.newArrayList(reader) could fix the warning.

@szehon-hoszehon-hoSep 3, 2021

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Good catch, done

openinx
openinx approved these changes Sep 3, 2021
View reviewed changes

@openinxopeninx left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks great to me ! Thanks @szehon-ho for the contribution, and @stevenzwu for the reviewing. I just left few minor comments.

@szehon-ho

szehon-ho commented Sep 3, 2021

Copy link
Copy Markdown
MemberAuthor

@openinx done, thanks for taking a look!

@openinx

openinx commented Sep 4, 2021

Copy link
Copy Markdown
Member

LGTM, thanks for the contribution!

@openinx
openinx merged commit 7ff9eb4 into apache:masterSep 4, 2021
@stevenzwustevenzwu mentioned this pull request Oct 19, 2021
Merged
@szehon-hoszehon-ho mentioned this pull request Oct 22, 2021
Closed
@zhongyujiangzhongyujiang mentioned this pull request Feb 14, 2022
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@stevenzwustevenzwustevenzwu approved these changes
@openinxopeninxopeninx approved these changes

Assignees

No one assigned

Labels

flink

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@szehon-ho@stevenzwu@openinx