Add pre-check for heavy debug log

[fectrain]

Fixes #15704

Description

Add pre-check for heavy debug log

Release note

This PR has:

• been self-reviewed.
• added documentation for new or modified features or behaviors.
• a release note entry in the PR description.
• added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
• added or updated version, license, or notice information in licenses.yaml
• added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
• added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.
• added integration tests.
• been tested in a test Druid cluster.

[kfaraz]

I suppose the changes in this class are not really needed as it is just a test class anyway. The changes wouldn't really have any impact.

[fectrain]

Okie, already reverted this one

[kfaraz]

Thanks for the changes @fectrain !

[kfaraz]

@fectrain , there seem to be a few code coverage issues. Please see if you can address them by adding /updating unit tests.

[fectrain]

Hello, @kfaraz! Thank you for your review and suggestions.

I'm also thinking about this but haven't found a straightforward solution to address this coverage issue. One option might be to enable debug-level logging for the entire unit test package through modifications to the log property files, which I think may not be quite necessary. 😄

As this change only involves adding a condition check and adjusting the indentation of the original log line, which should not able to alter the original logic or introduce exceptions. So I think merging this change is fine. What do you think?

[kfaraz]

@fectrain , yes, I agree that the changes here are simple enough to not cause any issue. But the problem is that it would also reduce the overall coverage of these classes because of the lines that are now inside the log.isDebugEnabled if clause. That might cause further build failures down the line.

Enabling debug logging in tests doesn't seem to be a great option either as it would create unnecessary logs when running tests.

See if you can reduce the number of lines in some cases. For example, the following code:

log.debug(                             
    "ZKNode created for server to [%s] %s [%s]",
    basePath,
    segmentHolder.getAction(),
    segmentHolder.getSegmentIdentifier()
);

can be rewritten as

log.debug(                             
    "ZKNode created for server to [%s] %s [%s]",
    basePath, segmentHolder.getAction(), segmentHolder.getSegmentIdentifier()
);

without compromising on readability.

I know it's not a great solution but should help with the coverage to some extent.

[fectrain]

Hi, @kfaraz, thanks for the advice.
I just inlined some logs as suggested to improve the line coverage rate, can see if now it's ok~

[kfaraz]

Thanks for your patience, @fectrain ! I have left some final comments. Please let me know if you would be available to address these. If not, I will commit the changes myself and merge the PR.

[FrankChen021]

Hi @fectrain Can you pick up this PR again to resolve the conflict so that we can merge it?

[fectrain]

oh, I might have missed this one, let me refine on it, thanks for reminding @FrankChen021

[fectrain]

Hi, @kfaraz , big thanks for all your advice on this. I've applied the changes you suggested and hope they're good to go for merging~ 🙏

[kfaraz]

The failing checks are all due to coverage of the new log lines. The coverage requirement is difficult to satisfy without enabling debug logging for all the relevant tests, which seems overkill.

Going forward with the merge.