Skip to content
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.

Sign up for GitHub

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

[ML] Reinstate ML daily maintenance actions #47103

Merged
merged 1 commit into from
Sep 26, 2019
Merged

[ML] Reinstate ML daily maintenance actions #47103

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/MlDailyMaintenanceService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,12 +79,12 @@ private static TimeValue delayToNextTime(ClusterName clusterName) {
return TimeValue.timeValueMillis(next.toInstant().toEpochMilli() - now.toInstant().toEpochMilli());
}

public void start() {
public synchronized void start() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose there is no harm in making this synchronized, it just seems unnecessary as all it does is call scheduleNext which is also synchronized.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it's redundant in terms of thread safety, but I thought it was worth making sure the logging order of "Starting ML daily maintenance service" and "Stopping ML daily maintenance service" matched the order that the corresponding operations were performed. Without this synchronized the log could say "Starting" followed by "Stopping", but actually the work of starting was done after the work of stopping.

LOGGER.debug("Starting ML daily maintenance service");
scheduleNext();
}

public void stop() {
public synchronized void stop() {
LOGGER.debug("Stopping ML daily maintenance service");
if (cancellable != null && cancellable.isCancelled() == false) {
cancellable.cancel();
Expand All @@ -100,7 +100,7 @@ public void close() {
stop();
}

private void scheduleNext() {
private synchronized void scheduleNext() {
try {
cancellable = threadPool.schedule(this::triggerTasks, schedulerProvider.get(), ThreadPool.Names.GENERIC);
} catch (EsRejectedExecutionException e) {
Expand Down
7 changes: 4 additions & 3 deletions x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/MlInitializationService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ class MlInitializationService implements LocalNodeMasterListener, ClusterStateLi
this.clusterService = clusterService;
this.client = client;
clusterService.addListener(this);
clusterService.addLocalNodeMasterListener(this);
}

@Override
Expand Down Expand Up @@ -80,7 +81,7 @@ public String executorName() {
return ThreadPool.Names.GENERIC;
}

private void installDailyMaintenanceService() {
private synchronized void installDailyMaintenanceService() {
if (mlDailyMaintenanceService == null) {
mlDailyMaintenanceService = new MlDailyMaintenanceService(clusterService.getClusterName(), threadPool, client);
mlDailyMaintenanceService.start();
Expand All @@ -93,7 +94,7 @@ public void beforeStop() {
}
}

private void uninstallDailyMaintenanceService() {
private synchronized void uninstallDailyMaintenanceService() {
if (mlDailyMaintenanceService != null) {
mlDailyMaintenanceService.stop();
mlDailyMaintenanceService = null;
Expand All @@ -106,7 +107,7 @@ MlDailyMaintenanceService getDailyMaintenanceService() {
}

/** For testing */
void setDailyMaintenanceService(MlDailyMaintenanceService service) {
synchronized void setDailyMaintenanceService(MlDailyMaintenanceService service) {
mlDailyMaintenanceService = service;
}
}
Expand Down