-
Notifications
You must be signed in to change notification settings - Fork 21
CNDB-15157: limit log lines during commitlog replay #2001
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
base: main
Are you sure you want to change the base?
Conversation
Checklist before you submit for review
|
if (skippedSSTables.size() <= maxSSTablesToLog) { | ||
logger.debug("Ignored commitLogIntervals from the following sstables: {}", skippedSSTables); | ||
} else { | ||
List<String> sample = new ArrayList<>(skippedSSTables).subList(0, maxSSTablesToLog); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
if the number of sstables is very big this is going to create a big ArrayList, maybe we can do it the same way you did in "construct": iterate and keep the first N
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point! Updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have left a small comment.
I don't think it is worth to add a unit test for this.
it would be pretty tricky to setup, I don't know if we have facilities to intercept the logs. I find it overkill
I thought about that too and came to the same conclusion. |
// Limit the number of SSTable names logged to prevent massive log lines | ||
int maxSSTablesToLog = 100; | ||
if (skippedSSTables.size() <= maxSSTablesToLog) { | ||
logger.debug("Ignored commitLogIntervals from the following sstables: {}", skippedSSTables); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was wondering: Do we always hit this in CNDB replayer? If yes, is there a way we can turn the log off if it's not useful?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we don't want them just in CNDB, we should be able to suppress them with logback. If you'd prefer that I can do it in a different PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually I think we'd have to suppress all the logs from CommitLogReplayer if we do it though logback.
|
✔️ Build ds-cassandra-pr-gate/PR-2001 approved by ButlerApproved by Butler |
What is the issue
Very long log lines can be produced during commitlog replay
What does this PR fix and why was it fixed
Limits the amount of information printed at debug, but retains full information at trace