[DNM] logpuller: optimize log puller performance under single large table scenario #3814
Conversation
ti-chi-bot
bot
added
do-not-merge/needs-linked-issue
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
ti-chi-bot
bot
added
the
size/XXL
Summary of ChangesHello @lidezhu, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request undertakes a significant refactoring of the event processing architecture within the log puller component. The primary goal is to enhance concurrency and modularity by introducing a dedicated Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a significant refactoring to the region event handling logic by replacing the single-threaded regionEventHandler with a concurrent regionEventProcessor that uses a pool of workers. This change aims to improve performance by processing region events in parallel. The logic from the old handler has been moved into the new processor and a new subscriptionEventHandler. The changes are well-structured and improve the design. I've found a potential build-breaking issue due to Go version compatibility and an opportunity for code simplification.
lidezhu
changed the title
ti-chi-bot
bot
removed
the
do-not-merge/work-in-progress
|
/gemini summary |
Summary of ChangesThis pull request significantly refactors the log puller's event processing architecture to enhance performance and scalability, particularly for large tables. It introduces a Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
|
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a significant and well-executed refactoring of the log puller's event handling logic to improve performance, especially for scenarios with a single large table. The introduction of a parallel regionEventProcessor with a worker pool is a solid approach to leverage more CPU resources for processing region events. The optimization to batch resolved-ts events is also a great improvement.
I'm particularly impressed with the fix in region_range_lock.go that corrects a race condition by changing RLock to Lock. This is a critical correctness improvement.
The new architecture separating region event processing from subscription event handling (via dynstream) is much cleaner and removes previous workarounds for deadlocks. The code is well-structured and the changes are logical. I have a couple of minor suggestions for improving maintainability by extracting magic numbers into constants. Overall, this is a high-quality contribution.
| if workerCount <= 0 { | ||
| workerCount = 1 | ||
| } | ||
| if queueSize <= 0 { | ||
| queueSize = 1024 | ||
| } |
There was a problem hiding this comment.
These default values for workerCount and queueSize are hardcoded. It would be better to define them as constants at the package level, for example, defaultRegionEventWorkerCount and defaultRegionEventWorkerQueueSize. This improves readability and makes it easier to manage these default values. Please also add the constant definitions at the package level.
| if workerCount <= 0 { | |
| workerCount = 1 | |
| } | |
| if queueSize <= 0 { | |
| queueSize = 1024 | |
| } | |
| if (workerCount <= 0) { | |
| workerCount = defaultRegionEventWorkerCount | |
| } | |
| if (queueSize <= 0) { | |
| queueSize = defaultRegionEventWorkerQueueSize | |
| } |
| if len(regionIDs) == 0 { | ||
| return nil, 0, nil | ||
| } | ||
| missingSample = make([]uint64, 0, 8) |
There was a problem hiding this comment.
The capacity 8 for missingSample is a magic number. It's good practice to define it as a named constant, for example const missingRegionSampleCapacity = 8, to improve code clarity and maintainability.
| missingSample = make([]uint64, 0, 8) | |
| missingSample = make([]uint64, 0, missingRegionSampleCapacity) |
|
@lidezhu: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
lidezhu
changed the title
What problem does this PR solve?
Issue Number: ref #3800
What is changed and how it works?
This pull request significantly refactors the log puller's event processing architecture to enhance performance and scalability, particularly for large tables. It introduces a
regionEventProcessorthat handles region-specific events concurrently using a sharded worker pool. This new processor acts as an intermediary, processing raw events from workers and transforming them into higher-levelsubscriptionEvents before they are fed into theDynamicStream. This design aims to optimize the event handling pipeline by distributing the processing load, enabling batching of resolved timestamps, and improving concurrency control, ultimately leading to a more efficient and maintainable system.Highlights
regionEventProcessorcomponent to handle region-specific events, decoupling this logic from theDynamicStream's direct handler. This processor acts as an intermediary, receiving raw region events from workers, processing them, and then dispatching higher-levelsubscriptionEvents to theDynamicStream.regionEventProcessorutilizes a sharded worker pool, distributing the processing of region events concurrently based on region ID. This design aims to improve throughput and efficiency in event handling, especially for scenarios with a single large table.DynamicStreamnow interacts with a newsubscriptionEventHandlerthat processes aggregatedsubscriptionEvents. This simplifies theDynamicStream's direct responsibilities and allows for more focused event handling, reducing overhead.UpdateLockedRangeStateHeapmethod inregion_range_lock.gonow uses an exclusiveLockinstead of aRLock, ensuring more robust and thread-safe concurrent updates to the locked range state.regionEventProcessornow supports batch processing of resolved timestamps for multiple regions, which can reduce the number of events pushed to theDynamicStreamand improve performance.Check List
Tests
Questions
Will it cause performance regression or break compatibility?
Do you need to update user documentation, design documentation or monitoring documentation?
Release note