Added quartz scheduler to schedule maintenance job. #17
Conversation
| setMaintenanceSchedule(cronExpression); | ||
| } | ||
|
|
||
| private String convertToCronExpression(long interval, TimeUnit timeUnit) { |
There was a problem hiding this comment.
not asking to change anything but https://github.com/shyiko/skedule?tab=readme-ov-file#format would give us support for schedules like "every 12 hours", all with a lot less code & complexity (literally ~10 lines https://github.com/shyiko/skedule?tab=readme-ov-file#example-scheduling-using-scheduledthreadpoolexecutor).
There was a problem hiding this comment.
removed quartz and switched to skedule library,
|
Testing |
| scheduleNextMaintenance(); | ||
| } | ||
|
|
||
| public void setMaintenanceMode(boolean enabled) { |
There was a problem hiding this comment.
this should be private to avoid misuse
| } | ||
|
|
||
| public void stopScheduledMaintenance() { | ||
| if (currentTask != null) { |
There was a problem hiding this comment.
this doesn't look thread-safe
There was a problem hiding this comment.
Added synchronization on object
| List<TableIdentifier> tables = catalog.listTables(namespace); | ||
| for (TableIdentifier tableIdent : tables) { | ||
| int expirationDays = DEFAULT_EXPIRATION_DAYS; | ||
| String configuredDays = config.get(Config.OPTION_SNAPSHOT_EXPIRATION_DAYS); |
There was a problem hiding this comment.
this should be better done in a constructor (with invalid value resulting in an exception)
| logger.info("Next maintenance scheduled for: {}", next); | ||
| } | ||
|
|
||
| public void setMaintenanceSchedule(String scheduleExpression) { |
There was a problem hiding this comment.
I'd suggest moving this to constructor (and making schedule final). Otherwise we have an issue with thread-safety here.
| @@ -276,6 +284,9 @@ public Integer call() throws Exception { | |||
|
|
|||
| Catalog catalog = CatalogUtil.buildIcebergCatalog("rest_backend", config, null); | |||
|
|
|||
| // Initialize and start the maintenance scheduler | |||
| initializeMaintenanceScheduler(catalog, config); | |||
There was a problem hiding this comment.
it would be nice to have maintenance optional (e.g. disable when maintenanceInterval is empty)
There was a problem hiding this comment.
logic added inside the function
private void initializeMaintenanceScheduler(Catalog catalog, Map<String, String> config) {
if (maintenanceInterval == null || maintenanceInterval.trim().isEmpty()) {
logger.info("Maintenance scheduler is disabled (no maintenance interval specified)");
return;
```
…tTask accessed from multiple threads.
…tTask accessed from multiple threads.
|
#17 (comment) may have been lost |
Sorry are u referring to the constructor comment, its done here |
closes: #1