-
Couldn't load subscription status.
- Fork 28
metrics: fix changefeed resolved ts #2693
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
Conversation
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 significantly refines the log service's ability to track and manage schema store states, particularly concerning resolved timestamps. By introducing per-changefeed tracking within the schema store and enabling it to report its progress to the log coordinator, the system achieves improved accuracy and granularity in monitoring changefeed replication. The coordinator's logic is updated to seamlessly incorporate these new schema store metrics, ensuring a more robust and precise calculation of the overall resolved timestamp for each changefeed. Highlights
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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a mechanism for schema stores to report their state to the log coordinator, which allows for more accurate changefeed metrics. The changes include updating Protobuf definitions, modifying the log coordinator to handle these new states, and implementing the reporting logic in the schema store. The overall approach is sound. I've identified a minor performance issue related to slice allocation in the coordinator and a more significant correctness bug in how schema store metrics are aggregated when a changefeed spans multiple keyspaces. Addressing these points will improve the robustness and performance of the new functionality.
65a0bd5 to
f5a7bb7
Compare
|
/test all |
|
/gemini summary |
Summary of ChangesThis pull request refactors the mechanism for tracking and reporting changefeed resolved timestamps within the system. It shifts the responsibility from the Highlights
Changelog
Activity
|
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request refactors the collection and reporting of changefeed resolved timestamps, moving the responsibility from eventStore to eventService. This is a sensible architectural change that simplifies the eventStore component. The implementation correctly relocates the logic and adjusts the necessary communication paths between the logCoordinator and the eventService.
I have identified a critical issue in logservice/schemastore/persist_storage.go where changing a panic to a silent return during initialization could lead to subsequent panics due to a partially initialized object. Additionally, I've offered a couple of medium-severity suggestions for the new logic in pkg/eventservice/metrics_collector.go to enhance code clarity and performance.
|
/test all |
|
/test pull-cdc-mysql-integration-light |
|
/retest |
|
/test pull-cdc-mysql-integration-light |
|
/test all |
|
/test all |
|
/test all |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: asddongmen, hongyunyan The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
[LGTM Timeline notifier]Timeline:
|
|
@lidezhu: The following tests failed, say
Full PR test history. Your PR dashboard. Instructions 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. |
|
/retest |
|
In response to a cherrypick label: new pull request created to branch |
What problem does this PR solve?
Issue Number: close #2694
What is changed and how it works?
eventStoreto theeventService'smetricsCollector.eventStoreno longer maintainschangefeedStatstructures or directly collects/reports changefeed-level metrics, simplifying its internal logic.eventServicenow aggregates minimum resolved timestamps per changefeed and reports them to the log coordinator more frequently (every 1 second).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