-
Notifications
You must be signed in to change notification settings - Fork 38
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
CASSSIDECAR-220: Add CdcRawDirectorySpaceCleaner for tracking and cleaning up the cdc_… #203
base: trunk
Are you sure you want to change the base?
Conversation
boolean enableCdcRawDirectoryRoutineCleanUp(); | ||
|
||
/** | ||
* @return fallback value for maximum directory size in bytes for the `cdc_raw` directory when can't be read from `system_views.settings` table |
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: final .
(dot)
String cdcTotalSpaceInMb = getSetting(YAML_PROP_PREVIOUS); | ||
if (cdcTotalSpaceInMb != null) | ||
{ | ||
return FileUtils.mbStringToBytes(cdcTotalSpaceInMb); |
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.
What do we mean by previous? Is this being deprecated? Maybe we should throw a deprecation warn if that's the case?
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.
cdc_total_space_in_mb
was removed in Cassandra and replaced with cdc_total_space
in v5.0+
periodicTaskExecutor.schedule(healthCheckPeriodicTask); | ||
periodicTaskExecutor.schedule(cdcRawDirectorySpaceCleaner); | ||
vertx.eventBus().localConsumer(ON_SERVER_STOP.address(), message -> |
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.
Please also unschedule the new cdcRawDirectorySpaceCleaner
periodic task
/** | ||
* @return the critical time period in seconds that indicates the `cdc_raw` directory is not large enough to buffer this time-window of mutations. | ||
*/ | ||
Duration cdcRawDirectoryCriticalBufferWindow(); |
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.
Why using Duration instead of SecondBoundConfiguration
? Same for cdcRawDirectoryLowBufferWindow
and cacheMaxUsage
if (!cdcConfiguration.enableCdcRawDirectoryRoutineCleanUp()) | ||
{ | ||
LOGGER.debug("Skipping CdcRawDirectorySpaceCleaner: feature is disabled "); | ||
return; | ||
} |
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.
Instead of having this check here, why not just prevent the task from being scheduled by overriding scheduleDecision
method? Something like:
@Override
public ScheduleDecision scheduleDecision()
{
return cdcConfiguration.enableCdcRawDirectoryRoutineCleanUp() ? ScheduleDecision.EXECUTE : ScheduleDecision.SKIP;
}
…raw directories