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

Fix unexpected max latency in output of stat #349

Merged
merged 2 commits into from
Sep 4, 2024

Conversation

lzydmxy
Copy link
Contributor

@lzydmxy lzydmxy commented Sep 3, 2024

Which issues of this PR fixes:

This PR try to fix #348

Change log:

# Conflicts:
#	src/Service/Metrics.h
Int64 elapsed = Poco::Timestamp().epochMicroseconds() / 1000 - response->request_created_time_ms;
auto current_time = getCurrentTimeMilliseconds();
Int64 elapsed = current_time - response->request_created_time_ms;
if (elapsed < 0)
Copy link
Contributor

@JackyWoo JackyWoo Sep 4, 2024

Choose a reason for hiding this comment

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

Why elapsed can be -1? Caused by different clock? If it is, we must use stedy_lock in all places.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe different clock, so I use getCurrentTimeMilliseconds which is steady_clock for request in all places.

Copy link
Contributor

@JackyWoo JackyWoo Sep 4, 2024

Choose a reason for hiding this comment

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

Maybe caused by clock diff in different servers. When leader append_entries the create_time is attached to the log and when the follower takes the log the timestamp in leader is used. If there is a clock diff in the servers -1 may raise.

Copy link
Contributor

@JackyWoo JackyWoo Sep 4, 2024

Choose a reason for hiding this comment

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

Let's add a comment to help readers understand the code.

Copy link
Contributor Author

@lzydmxy lzydmxy Sep 4, 2024

Choose a reason for hiding this comment

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

Maybe caused by clock diff in different servers. When leader append_entries the create_time is attached to the log and when the follower takes the log the timestamp in leader is used. If there is a clock diff in the servers -1 may raise.

When followers takes the log from leader, them won't updateLatency for the log. We used to use system_clock to measure request time cost. Clock rollback can cause this issue with system_clock.

@JackyWoo JackyWoo added the bug Something isn't working label Sep 4, 2024
@JackyWoo JackyWoo added this to the Release v2.2.0 milestone Sep 4, 2024
@JackyWoo JackyWoo changed the title Fix error max latency Fix unexpected max latency in output of stat Sep 4, 2024
response->xid,
Coordination::toString(response->getOpNum()),
elapsed);
LOG_WARNING(
Copy link
Contributor

@JackyWoo JackyWoo Sep 4, 2024

Choose a reason for hiding this comment

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

Just delete the logging, for it is confused for the system mantainers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay

@JackyWoo JackyWoo merged commit 97d58cd into JDRaftKeeper:master Sep 4, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix unexpected max latency in output of stat
2 participants