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

[HUDI-4717] CompactionCommitEvent message corrupted when sent by compact_task #6524

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nonggialiang
Copy link

…act_task

Change Logs

Use mail-box executor to send compaction-commit-event in CompactFunction.

Impact

Potential bug fixed.

Risk level: none | low | medium | high

Choose one. If medium or high, explain what verification was done to mitigate the risks.

Contributor's checklist

  • Read through contributor's guide
  • Change Logs and Impact were stated clearly
  • Adequate tests were added if applicable
  • CI passed

@yihua yihua requested a review from danny0405 August 30, 2022 05:42
@yihua yihua added priority:major degraded perf; unable to move forward; potential bugs flink Issues related to flink table-service labels Aug 30, 2022
private void sendEventByMailBox(Collector<CompactionCommitEvent> collector, CompactionCommitEvent commitEvent) {
mailboxExecutorAdapter.execute(() -> collector.collect(commitEvent),
"Send compaction commit event by mailbox.");
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, we may need to figure out more simpler way for collector concurrency.

Copy link
Author

Choose a reason for hiding this comment

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

Just pushed a new implementaion using AsynFunction of flink.
And fixed the same potential problem in clustering operator.

@hudi-bot
Copy link

CI report:

Bot commands @hudi-bot supports the following commands:
  • @hudi-bot run azure re-run the last Azure build

@nsivabalan nsivabalan added the release-0.12.2 Patches targetted for 0.12.2 label Dec 6, 2022
@codope
Copy link
Member

codope commented Dec 7, 2022

@nonggialiang Can you please rebase?

@littleeleventhwolf
Copy link
Contributor

@nonggialiang After using AsyncFunction to rewrite the asynchronous compaction logic, have you solved the problem described in HUDI-4717

@danny0405
Copy link
Contributor

danny0405 commented Jan 3, 2023

Does #7399 solve your problem here ?

@littleeleventhwolf
Copy link
Contributor

Does #7399 solve your problem here ?

Yeah, we cherry-pick #7399. But if user enable latency-marker, there is still thread-safety problem.

@danny0405
Copy link
Contributor

Does #7399 solve your problem here ?

Yeah, we cherry-pick #7399. But if user enable latency-marker, there is still thread-safety problem.

Does the Operator has any interface to handle the latency-marker yet ? Just like what we do to watermark.

@littleeleventhwolf
Copy link
Contributor

Does #7399 solve your problem here ?

Yeah, we cherry-pick #7399. But if user enable latency-marker, there is still thread-safety problem.

Does the Operator has any interface to handle the latency-marker yet ? Just like what we do to watermark.

We can provide an empty method processLatencyMarker(LatencyMarker latencyMarker) to not propagate the latency-marker. But I don't think this is an elegant way, for two reasons:
1) if Flink generates a new StreamElement in future version, we also need to provide an empty method for compatibility;
2) if users want to use Watermark or LatencyMarker do some other work in Operator Sink:compact_commit, an empty method that does not propagate these events cannot support users' requirement.

@github-actions github-actions bot added the size:M PR with lines of changes in (100, 300] label Feb 26, 2024
@yihua
Copy link
Contributor

yihua commented Mar 26, 2024

@danny0405 is this PR still useful?

@danny0405
Copy link
Contributor

yeah, just need to reach concensus.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
flink Issues related to flink priority:major degraded perf; unable to move forward; potential bugs release-0.12.2 Patches targetted for 0.12.2 size:M PR with lines of changes in (100, 300] table-service
Projects
Status: 🚧 Needs Repro
Status: 🔖 Ready for review
Development

Successfully merging this pull request may close these issues.

7 participants