GCInspector should use different thresholds on GC events #4433
Conversation
| logger.info(sb.toString()); | ||
| else if (logger.isTraceEnabled()) | ||
| logger.trace(sb.toString()); | ||
| // TODO: trigger StatusLogger if concurrent phases take too long? |
There was a problem hiding this comment.
regarding the TODO, yes, I think it makes sense to do a symmetric logic: too long GC can steal some computation resources from request processing and it maybe helpful to flush stats to see a possible impact.
Note: the threshold for status logging should be correspondent (based on concurrent phase log thresholds)
There was a problem hiding this comment.
should be this comment removed as I see this patch contains StatusLogger.log already?
There was a problem hiding this comment.
we have it for non-concurrent branch only, so it is actual for the current branch with a concurrent threshold logic
|
@Yukei7 could you please clarify have you tried to test the logic e2e manually (by running a Cassandra node built from the branch using ZGC or Shenandoah GC)? I understand that an automatic test would take too much efforts to build, so the question is only about a manual check to ensure that e2e works in the expected way. |
|
|
||
| public void setGcConcurrentPhaseWarnThresholdInMs(int threshold) | ||
| { | ||
| long gcConcurrentPhaseLogThresholdInMs = getGcConcurrentPhaseLogThresholdInMs(); |
There was a problem hiding this comment.
you can move the resolution of this after the first "if". I that throws you have called this unnecessarily.
You have already done it like that in setGcConcurrentPhaseLogThresholdInMs
| return DatabaseDescriptor.getGCConcurrentPhaseLogThreshold(); | ||
| } | ||
|
|
||
| public void setGcConcurrentPhaseLogThresholdInMs(int threshold) |
There was a problem hiding this comment.
I get that what you did here just follows the style which was already there and there is nothing wrong with it as such but I just wonder if we could not move the logic of this into DD's method and this one would just call it. I get that the methods need to be here because we are exposing them in MBean but the logic itself might be just moved to DD, no?
Because if somebody every calls DD methods, there is no validation. But it is just here. So everybody has to go through this method to get the validation. If we moved it to DD then it would be validated every time, regardless from where it is called.
DD currently also contains methods which have some basic validation around its parameters so ...
There was a problem hiding this comment.
also as I understand it .... what happens when you put into yaml logically invalid values and you start the node? because the validation occurs only when you call mbean methods. If we want to be sure that what we started the node with makes sense already then it would need to be validated in DatabaseDesciptor.applySimpleConfig that is called as part of node startup and validated there so we are sure it is all valid already
| public void ensureWarnGreaterThanLog() | ||
| { | ||
| gcInspector.setGcWarnThresholdInMs(gcInspector.getGcLogThresholdInMs()); | ||
| try |
There was a problem hiding this comment.
Better style is to use Assertions.assertThrownBy
| assertEquals(1000, gcInspector.getGcConcurrentPhaseLogThresholdInMs()); | ||
| gcInspector.setGcConcurrentPhaseWarnThresholdInMs(2000); | ||
| assertEquals(2000, gcInspector.getGcConcurrentPhaseWarnThresholdInMs()); | ||
| try |
There was a problem hiding this comment.
Assertions.assertThrownBy
3 participants
The Cassandra Jira