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

ZOOKEEPER-3061: add more details to 'Unhandled scenario for peer' log.warn message #555

Closed
wants to merge 3 commits into from

Conversation

cpoerschke
Copy link
Contributor

No description provided.

Copy link
Contributor

@nkalmar nkalmar left a comment

Choose a reason for hiding this comment

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

LGTM code wise, jenkins failed due to a flaky test. - Please re run with an empty amend commit for example.

As already mentioned in the jira, the same log is already there on INFO level. I've been warned before for duplicate logs, but seems like it could make debugging from logs easier.

@@ -792,7 +792,14 @@ public boolean syncFollower(long peerLastZxid, ZKDatabase db, Leader leader) {
txnProposalItr.close();
}
} else {
LOG.warn("Unhandled scenario for peer sid: " + getSid());
LOG.warn("Unhandled scenario for peer sid: {} maxCommittedLog=0x{}"
Copy link
Contributor

Choose a reason for hiding this comment

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

The same context is also provided at line 703 except the txnLogSyncEnabled, looks like nothing changed between these lines, any reason we added this extra logging here?

Copy link
Contributor

Choose a reason for hiding this comment

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

the logging levels are different, and it is nice to have the evaluation information with weird corner case scenario. i think it's worth surfacing the information here as well.

Copy link
Contributor

@breed breed left a comment

Choose a reason for hiding this comment

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

+1 i think this is useful for problem determination. especially when running with WARNING level.

@@ -792,7 +792,14 @@ public boolean syncFollower(long peerLastZxid, ZKDatabase db, Leader leader) {
txnProposalItr.close();
}
} else {
LOG.warn("Unhandled scenario for peer sid: " + getSid());
LOG.warn("Unhandled scenario for peer sid: {} maxCommittedLog=0x{}"
Copy link
Contributor

Choose a reason for hiding this comment

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

the logging levels are different, and it is nice to have the evaluation information with weird corner case scenario. i think it's worth surfacing the information here as well.

@asfgit asfgit closed this in 726587e Jul 28, 2018
RokLenarcic pushed a commit to RokLenarcic/zookeeper that referenced this pull request Sep 3, 2022
….warn message

Author: Christine Poerschke <[email protected]>
Author: Christine Poerschke <[email protected]>

Reviewers: Allan Lyu <[email protected]>, Benjamin Reed <[email protected]>, Norbert Kalmar <[email protected]>

Closes apache#555 from cpoerschke/master-ZOOKEEPER-3061 and squashes the following commits:

1399682 [Christine Poerschke] Merge remote-tracking branch 'origin/master' into master-ZOOKEEPER-3061
1840c2b [Christine Poerschke] Merge remote-tracking branch 'origin/master' into master-ZOOKEEPER-3061
1d3e7bc [Christine Poerschke] ZOOKEEPER-3061: add more details to 'Unhandled scenario for peer' log.warn message
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