Skip to content

Flink: Maintenance - Lock remover#11010

Merged
pvary merged 4 commits into
apache:mainfrom
pvary:lock_remover
Sep 12, 2024
Merged

Flink: Maintenance - Lock remover#11010
pvary merged 4 commits into
apache:mainfrom
pvary:lock_remover

Conversation

@pvary
Copy link
Copy Markdown
Contributor

@pvary pvary commented Aug 26, 2024

@github-actions github-actions Bot added the flink label Aug 26, 2024
@pvary pvary requested a review from stevenzwu August 26, 2024 18:20
duration,
taskResult.success(),
taskResult.exceptions())));
lock.unlock();
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.

I don't quite follow the logic.

the lock is always unlocked in both processElement and processWatermark, when will this lock will be locked, in the TriggerManager?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

You can check the Locking paragraph in the design doc: https://docs.google.com/document/d/16g3vR18mVBy8jbFaLjf2JwAANuYOmIwr15yDDxovdnA

If you still have questions, feel free to ask

@pvary
Copy link
Copy Markdown
Contributor Author

pvary commented Sep 11, 2024

@stevenzwu: I didn't get answer for my question on the Flink dev list about the output of the PostCommitTopology. See: https://lists.apache.org/thread/28qb1q6b7kz30dqjdbsw855osx4t38s7

I created some unit test and checked that if we return Void the PostCommitTopology is executed.

After some back-and-forth myself, I decided to move forward with the Void output as suggested by you. Mainly because of the LockRemover will be used in 2 cases:

  1. Post Commit Maintenance - in this case the output could not be used
  2. Separate Maintenance Job - in this case the output could be used to act upon failures, but it is unlikely and could be implemented if required later

The LockRemover will not be used when External scheduler is used.

Please share your thoughts, and review.
Thanks,
Peter

@pvary pvary merged commit 6ff7a6e into apache:main Sep 12, 2024
@pvary pvary deleted the lock_remover branch September 12, 2024 05:18
@pvary
Copy link
Copy Markdown
Contributor Author

pvary commented Sep 12, 2024

Merged to main.
Thanks for the review @stevenzwu and @advancedxy!

pvary pushed a commit to pvary/iceberg that referenced this pull request Sep 12, 2024
pvary added a commit that referenced this pull request Sep 12, 2024
zachdisc pushed a commit to zachdisc/iceberg that referenced this pull request Dec 23, 2024
zachdisc pushed a commit to zachdisc/iceberg that referenced this pull request Dec 23, 2024
czy006 pushed a commit to czy006/iceberg that referenced this pull request Apr 2, 2025
czy006 pushed a commit to czy006/iceberg that referenced this pull request Apr 2, 2025
czy006 added a commit to czy006/iceberg that referenced this pull request Apr 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants