You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2022/05/16 10:25:30 UTC

[GitHub] [iceberg] nastra opened a new pull request, #4786: [0.13] Flink upsert delete file metadata backports

nastra opened a new pull request, #4786:
URL: https://github.com/apache/iceberg/pull/4786

   This backports #4417 / #3834 (because #4364 depends on it) / #4364 / #4189 (because CI was failing without this test fix) to the 0.13.x branch


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org

[GitHub] [iceberg] kbendick commented on pull request #4786: [0.13] Flink upsert delete file metadata backports

Posted by GitBox <gi...@apache.org>.
kbendick commented on PR #4786:
URL: https://github.com/apache/iceberg/pull/4786#issuecomment-1129240482

   I'm not sure what's the deal with Flink 1.13 tests. Maybe the heap change needs to be merged in _first_? It seems though that the 1.13 tests are just... waiting.
   
   ![image](https://user-images.githubusercontent.com/9833362/168894952-7fdbad70-ac4b-4b88-87a8-b61cafc88a09.png)
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org

[GitHub] [iceberg] kbendick commented on pull request #4786: [0.13] Flink upsert delete file metadata backports

Posted by GitBox <gi...@apache.org>.
kbendick commented on PR #4786:
URL: https://github.com/apache/iceberg/pull/4786#issuecomment-1129601131

   Oh and the other thing would be to add class unloading into the test JVM settings. I helped tune the JVM for CI in Spark and class unloading, a fixed MaxMetaSpaceSize (to force class unloading), and somewhat larger heaps helped quite a bit for a situation that had gotten really out of hand-
   
   Also my only meaningful contribution to the Spark repo and the credit got misattributed as I never added that email to my GitHub 😂


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org

[GitHub] [iceberg] nastra commented on pull request #4786: [0.13] Flink upsert delete file metadata backports

Posted by GitBox <gi...@apache.org>.
nastra commented on PR #4786:
URL: https://github.com/apache/iceberg/pull/4786#issuecomment-1127533158

   @kbendick could you please double-check if there are any other Flink changes that need to be backported?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org

[GitHub] [iceberg] rdblue merged pull request #4786: [0.13] Flink upsert delete file metadata backports

Posted by GitBox <gi...@apache.org>.
rdblue merged PR #4786:
URL: https://github.com/apache/iceberg/pull/4786


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org

[GitHub] [iceberg] nastra commented on pull request #4786: [0.13] Flink upsert delete file metadata backports

Posted by GitBox <gi...@apache.org>.
nastra commented on PR #4786:
URL: https://github.com/apache/iceberg/pull/4786#issuecomment-1129596652

   Yes the job is timing out but it's unclear why because the run itself doesn't show any output. Locally it helped to increase the Heap size for Gradle (since the build didn't progress as well locally)


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org

[GitHub] [iceberg] kbendick commented on pull request #4786: [0.13] Flink upsert delete file metadata backports

Posted by GitBox <gi...@apache.org>.
kbendick commented on PR #4786:
URL: https://github.com/apache/iceberg/pull/4786#issuecomment-1132133637

   > I think I found the issue (which can easily be debugged locally by adding `--no-parallel --debug` to get `./gradlew -DsparkVersions= -DhiveVersions= -DflinkVersions=1.13 :iceberg-flink:iceberg-flink-1.13:check :iceberg-flink:iceberg-flink-runtime-1.13:check -Pquick=true -x javadoc --no-parallel --debug`). It looks like something got messed up during conflict resolution and `BaseDeltaTaskWriter#asStructLike(..)` was accidentally returning `keyWrapper.wrap(data)` and not `wrapper.wrap(data)`. This then resulted in `TestChangeLogTable#testChangeLogOnDataKey()` to run indefinitely in a retry-loop because there was an underlying exception. For the future I think it would be good to have a timeout on tests as issues like these are quite difficult to debug on CI without modifying how the test is being executed (e.g. by adding `--debug` to the gradle task and such)
   
   It's possible there's somethign else we need to cherry-pick given the the underlying exception for that one test case. I'll run locally and we'll see.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org

[GitHub] [iceberg] kbendick commented on pull request #4786: [0.13] Flink upsert delete file metadata backports

Posted by GitBox <gi...@apache.org>.
kbendick commented on PR #4786:
URL: https://github.com/apache/iceberg/pull/4786#issuecomment-1129599484

   So GH Actions use the settings in master for security purposes.
   
   Mayhe merging in the heap increase _first_ will help? I doubt the tests are running with the heap increase if it’s in this PR. Just a guess but the tests didn’t complete without increased heap for me as well.
   
   Also, since this is a release branch, feel free to make the heap slightly bigger and/or tune the forked JVM’s GC settings in the settings.gradle (or wherever).
   
   But I think running with the heap settings already merged in will help the most. I think you can try it in your fork if you run the tests there.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org

[GitHub] [iceberg] kbendick commented on pull request #4786: [0.13] Flink upsert delete file metadata backports

Posted by GitBox <gi...@apache.org>.
kbendick commented on PR #4786:
URL: https://github.com/apache/iceberg/pull/4786#issuecomment-1129604491

   The only other issue I can think of is a GitHub issue with the JVM cache as the problem is sticky for Flink 1.13.
   
   But the most recent attempt _did_ run somewhat so I don’t think that’s it. I think it’s just poor memory utilization / availability combined with some somewhat heavy Flink SQL upsert tests. 


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org

[GitHub] [iceberg] nastra commented on a diff in pull request #4786: [0.13] Flink upsert delete file metadata backports

Posted by GitBox <gi...@apache.org>.
nastra commented on code in PR #4786:
URL: https://github.com/apache/iceberg/pull/4786#discussion_r873601111


##########
flink/v1.12/flink/src/main/java/org/apache/iceberg/flink/sink/BaseDeltaTaskWriter.java:
##########
@@ -103,5 +103,10 @@ protected class RowDataDeltaWriter extends BaseEqualityDeltaWriter {
     protected StructLike asStructLike(RowData data) {
       return wrapper.wrap(data);
     }
+
+    @Override
+    protected StructLike asStructLikeKey(RowData data) {
+      return wrapper.wrap(data);

Review Comment:
   @kbendick should this throw an exception like in https://github.com/apache/iceberg/blob/910f2712c7b731181c4a72fb6b8c66addeb76dd9/flink/v1.12/flink/src/main/java/org/apache/iceberg/flink/sink/BaseDeltaTaskWriter.java#L109 or does it even matter what we return here for Flink 1.12?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org

[GitHub] [iceberg] kbendick commented on a diff in pull request #4786: [0.13] Flink upsert delete file metadata backports

Posted by GitBox <gi...@apache.org>.
kbendick commented on code in PR #4786:
URL: https://github.com/apache/iceberg/pull/4786#discussion_r874200441


##########
flink/v1.12/flink/src/main/java/org/apache/iceberg/flink/sink/BaseDeltaTaskWriter.java:
##########
@@ -103,5 +103,10 @@ protected class RowDataDeltaWriter extends BaseEqualityDeltaWriter {
     protected StructLike asStructLike(RowData data) {
       return wrapper.wrap(data);
     }
+
+    @Override
+    protected StructLike asStructLikeKey(RowData data) {
+      return wrapper.wrap(data);

Review Comment:
   I would keep it this way upon initial inspection.
   
   It needs to be implemented for the API and it will possibly get called. Keeping it the same as `asStructLike` seems safest for backwards compatibility.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org

[GitHub] [iceberg] nastra commented on pull request #4786: [0.13] Flink upsert delete file metadata backports

Posted by GitBox <gi...@apache.org>.
nastra commented on PR #4786:
URL: https://github.com/apache/iceberg/pull/4786#issuecomment-1129858628

   I think I found the issue (which can easily be debugged locally by adding `--no-parallel --debug` to get `./gradlew -DsparkVersions= -DhiveVersions= -DflinkVersions=1.13 :iceberg-flink:iceberg-flink-1.13:check :iceberg-flink:iceberg-flink-runtime-1.13:check -Pquick=true -x javadoc --no-parallel --debug`). It looks like something got messed up during conflict resolution and `BaseDeltaTaskWriter#asStructLike(..)` was accidentally returning `keyWrapper.wrap(data)` and not `wrapper.wrap(data)`.
   This then resulted in `TestChangeLogTable#testChangeLogOnDataKey()` to run indefinitely in a retry-loop because there was an underlying exception. 
   For the future I think it would be good to have a timeout on tests as issues like these are quite difficult to debug on CI without modifying how the test is being executed (e.g. by adding `--debug` to the gradle task and such)
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org

[GitHub] [iceberg] rdblue commented on pull request #4786: [0.13] Flink upsert delete file metadata backports

Posted by GitBox <gi...@apache.org>.
rdblue commented on PR #4786:
URL: https://github.com/apache/iceberg/pull/4786#issuecomment-1129273274

   The test issue looks like a github problem. I tried to cancel, but it looks like I can't right now. I'll check back in a few hours. It should time out.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org