Skip to content

Zookeeper-3735: fix the bad format of RATE_LOGGER#1275

Closed
jonomorris wants to merge 6 commits intoapache:masterfrom
jonomorris:ZOOKEEPER-3735
Closed

Zookeeper-3735: fix the bad format of RATE_LOGGER#1275
jonomorris wants to merge 6 commits intoapache:masterfrom
jonomorris:ZOOKEEPER-3735

Conversation

@jonomorris
Copy link
Contributor

The message written to the Rate-logger was modified to conform with the way the Rate-logger composes messages it writes to the underlying logger.

RateLogger updates:

  1. Added Javadoc and Unit-tests.
  2. Updated code to omit logging the value when none is provided, and to prevent a previously logged value from being re-written after a flush.

image

Copy link
Member

@maoling maoling left a comment

Choose a reason for hiding this comment

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

I also found other two nits: we need to log zxid with Hex. Could you plz also fix them in passing:)
RATE_LOGGER.rateLimitLog("Digests are not matching. Value is Zxid.", String.valueOf(zxid));

if (now - timestamp >= LOG_INTERVAL) {
// log previous message and value
flush();
msg = newMsg;
Copy link
Member

Choose a reason for hiding this comment

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

Why do you need this change? Time interval has elapsed, reset count?

Copy link
Contributor Author

@jonomorris jonomorris Mar 8, 2020

Choose a reason for hiding this comment

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

If the time interval has elapsed call flush writing the existing value and current message count then save the message with new value provided as the first message received after the flush with count 1.

This change makes it consistent with an explicit flush, where the flush method is called on the RateLogger object, which writes the current message along with exiting value and current message count.

Copy link
Member

Choose a reason for hiding this comment

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

  • Haha, not very persuasive for me:).
  • And for test, when time interval has elapsed and at the same time, flush() is called explicitly, the tests will become complicated.
e.g:
A Permutation and Combination of these three lines
currentTime.set(DEFAULT_LOGGING_INTERVAL + 1L);
rateLogger.rateLimitLog("test message one.", "value-four");
rateLogger.flush();

Copy link
Contributor Author

@jonomorris jonomorris Mar 9, 2020

Choose a reason for hiding this comment

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

Hey @maoling, yes sorry there's a bit going on in that last unit test. I can simplify it to just test the flush after setting the elapsed time:

currentTime.set(DEFAULT_LOGGING_INTERVAL + 1L);

I guess I just wanted to demonstrate that how the message and value are retained after the message is flushed when the logging-interval is exceeded.

Did you want this bit of code in RateLogger reverted?

Copy link
Member

Choose a reason for hiding this comment

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

@jono-morris
Take it easy, let's wait here for other people's review and comment

rateLogger.rateLimitLog("test message one", "value-two");
rateLogger.rateLimitLog("test message one", "value-three");
Thread.sleep(2 * rateLogInterval);
// same message after rate interval triggers implicit flush
Copy link
Member

Choose a reason for hiding this comment

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

this test may be flaky, you can look at other example how to mock timing (e.g.: CreateContainerTest#testMaxNeverUsedInterval)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated RateLogger to get current time from a method with package access that is overridden during unit testing. This allows the time to be explicitly set similar to the way you have suggested.

@jonomorris
Copy link
Contributor Author

Also updated RateLogger calls in reportDigestMismatch and reportDigestMismatch methods to use
"0x" + Long.toHexString(zxid)

@jonomorris jonomorris closed this Feb 22, 2021
@jonomorris jonomorris reopened this Feb 22, 2021
@jonomorris
Copy link
Contributor Author

Not required

@jonomorris jonomorris closed this Feb 22, 2021
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

Comments