dlg99 commented on a change in pull request #851: Issue 578 : make
MajorCompaction controlled by time of the day/day of the week
URL: https://github.com/apache/bookkeeper/pull/851#discussion_r159505503
##########
File path:
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/GarbageCollectorThread.java
##########
@@ -264,12 +278,53 @@ public void safeRun() {
// collection cycle started
forceGarbageCollection.set(false);
}
+
+ }
+
+ /**
+ * increase the threshold when the system in low load status.
+ */
+ private void activateMedianThreshold(){
+ majorCompactionThreshold = medianMajorCompactionThreshold;
+ }
+ /**
+ * increase the threshold when the system in a very low load status.
+ */
+ private void activateHighThreshold(){
+ majorCompactionThreshold = highMajorCompactionThreshold;
+ }
+
+ /**
+ * Check whether the configured cron is met and activate threshold.
+ */
+ private void changeMajorCompactionThreshold(){
+ // next fire time is in the interval
+ if
(medianMajorCron.nextTimeAfter(ZonedDateTime.now()).toInstant().toEpochMilli()
Review comment:
Rather general notes after looking at this and the change overall. I can be
wrong and often am.
1. reusing cron/time-based parameter change for other parameters looks like
a royal pain in the bottom. It is not transparent part of the configuration.
Got to add extra parameter for the cron, one parameter for each cron.
Want to add extra interval? do the code change or get really creative with
cron definitions.
Want to do something similar for i.e. auditor configuration (i.e. to disable
it for maintenance windows), change compaction throttling, change journal's
buffer size or something else? Got to add configs for crons + replicate the
same logic of using the crons.
2. Cron is great for defining scheduled tasks/timers. It is confusing when
applied to interval ranges.
Imagine you have a prod issue at midnight of Friday potentially related to
compaction/disk load and you are trying to figure out which values these config
parameters had at exactly 23:42 GMT this Friday?
3. if we want cron, maybe make it actual cron. schedule a task that will run
and change any config parameter value. All that code has to do after this is to
read config value before using it rather than using the final value read once
in constructor.
This can be internal cron or simply external that changes config via rest
api (insert security considerations here)
internal cron can be configured like
```text
cron1 = 00 09-18 * * * isAuditorPeriodicCheckOkToRunNow=true,
auditorPeriodicBookieCheckInterval=86400, majorCompactionThreshold=0.6
cron1 = 00 11,16 * * * isAuditorPeriodicCheckOkToRunNow=false,
majorCompactionThreshold=0.8
```
it does not address p.2 but maybe logging cron events and parameters will
suffice.
Overall yes, it does cover that one usecase but I'd love to be able to
easily reuse cron functionality for other cases since we are adding code to
deal with crons. I think this will push the change from "ok" to "awesome".
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
With regards,
Apache Git Services
