Skip to content

Added log messages to debug assignPartitions#951

Merged
jzakaryan merged 2 commits intomasterfrom
jzakarya/logs-for-perf-debugging
Aug 7, 2023
Merged

Added log messages to debug assignPartitions#951
jzakaryan merged 2 commits intomasterfrom
jzakarya/logs-for-perf-debugging

Conversation

@jzakaryan
Copy link
Copy Markdown
Collaborator

We observed that the leader has a tendency to get stuck on this method (in some rare cases for more than 5 minutes). Added log messages to help debug the issue.

Note that the test suite in TestLoadBasedPartitionAssigner do not capture such performance problems. Attempts to reproduce them locally didn't work.

ehoner
ehoner previously approved these changes Aug 4, 2023
Copy link
Copy Markdown
Contributor

@ehoner ehoner left a comment

Choose a reason for hiding this comment

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

I am approving these in case they are time-sensitive. But in general I agree with @hshukla's comments. I am not convinced these changes will surface the issue(s), but this is a worthwhile first step before considering more involved changes.

@shrinandthakkar
Copy link
Copy Markdown
Collaborator

I remember us talking about putting logs around any IO operations that might be happening, should we add some logs there as well?

@jzakaryan
Copy link
Copy Markdown
Collaborator Author

I remember us talking about putting logs around any IO operations that might be happening, should we add some logs there as well?

The consensus at the end of the meeting was that the assignPartitions method is where the CPU time is spent. I avoided adding logs in other places as to keep unnecessary logging to minimum.

@jzakaryan jzakaryan requested a review from hshukla August 4, 2023 17:56
@jzakaryan jzakaryan merged commit 886c4c6 into master Aug 7, 2023
@hshukla hshukla deleted the jzakarya/logs-for-perf-debugging branch August 7, 2023 21:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants