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 server: fix write to slowlog on squashing flow #4138

Merged
merged 1 commit into from
Nov 17, 2024

Conversation

adiholden
Copy link
Collaborator

@adiholden adiholden commented Nov 17, 2024

The bug:
slow invocations where not written to slowlog
The fix:
extract the relevant connection pointer from context on squashing

Signed-off-by: adi_holden <adi@dragonflydb.io>
@adiholden adiholden requested a review from romange November 17, 2024 09:19
@@ -1358,6 +1358,10 @@ bool Service::InvokeCmd(const CommandId* cid, CmdArgList tail_args, SinkReplyBui
// TODO: we should probably discard more commands here,
// not just the blocking ones
const auto* conn = cntx->conn();
if (cntx->conn_state.squashing_info) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

what is the reason we dont save the connection pointer in cntx when running squashed commands?

Copy link
Collaborator

Choose a reason for hiding this comment

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

i dunno. maybe @dranikpg remembers.

@romange
Copy link
Collaborator

romange commented Nov 17, 2024

how it affects slow invocations? in other words why conn pointer is relevant to slowlog?

@adiholden
Copy link
Collaborator Author

how it affects slow invocations? in other words why conn pointer is relevant to slowlog?

we have this condition conn != nullptr in which we do not enter the slowlog checks if conn is null.
But I cant tell which flows have conn = nullptr

@adiholden adiholden merged commit 59c81fb into main Nov 17, 2024
12 checks passed
@adiholden adiholden deleted the fix_slowlog_on_squashing branch November 17, 2024 14:03
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.

2 participants