You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by GitBox <gi...@apache.org> on 2022/07/25 23:09:44 UTC
[GitHub] [accumulo] ctubbsii commented on a diff in pull request #2778: Add version check to ZooPropStore
ctubbsii commented on code in PR #2778:
URL: https://github.com/apache/accumulo/pull/2778#discussion_r929378827
##########
server/base/src/main/java/org/apache/accumulo/server/conf/ServerConfigurationFactory.java:
##########
@@ -140,4 +158,114 @@ public void connectionEvent() {
// no-op. changes handled by prop store impl
}
}
+
+ private class ConfigRefreshRunner {
+ private static final long MIN_JITTER_DELAY = 1;
+
+ private static final long MAX_JITTER_DELAY = 23;
+ private final ScheduledFuture<?> refreshTaskFuture;
+
+ ConfigRefreshRunner() {
+
+ Runnable refreshTask = this::verifySnapshotVersions;
+ // staggering the initial delay prevents synchronization of Accumulo servers communicating
+ // with ZooKeeper for the sync process.
+ long randDelay = jitter(REFRESH_PERIOD_MINUTES / 4, REFRESH_PERIOD_MINUTES);
+
+ ScheduledThreadPoolExecutor executor = ThreadPools.getServerThreadPools()
+ .createScheduledExecutorService(4, "config-refresh", false);
+ refreshTaskFuture =
+ executor.scheduleWithFixedDelay(refreshTask, randDelay, REFRESH_PERIOD_MINUTES, MINUTES);
+ }
+
+ /**
+ * Check that the stored version in ZooKeeper matches the version held in the local snapshot.
+ * When a mismatch is detected, a change event is sent to the prop store which will cause a
+ * re-load. If the Zookeeper node has been deleted, the local cache entries are removed.
+ * <p>
+ * This method is designed to be called as a scheduled task, so it does not propagate exceptions
+ * other than interrupted Exceptions so the scheduled tasks will continue to run.
+ */
+
+ private synchronized void verifySnapshotVersions() {
Review Comment:
I think this synchronized isn't helping unless you make this method static.
##########
server/base/src/main/java/org/apache/accumulo/server/conf/ServerConfigurationFactory.java:
##########
@@ -140,4 +158,114 @@ public void connectionEvent() {
// no-op. changes handled by prop store impl
}
}
+
+ private class ConfigRefreshRunner {
+ private static final long MIN_JITTER_DELAY = 1;
+
+ private static final long MAX_JITTER_DELAY = 23;
+ private final ScheduledFuture<?> refreshTaskFuture;
+
+ ConfigRefreshRunner() {
+
+ Runnable refreshTask = this::verifySnapshotVersions;
+ // staggering the initial delay prevents synchronization of Accumulo servers communicating
+ // with ZooKeeper for the sync process.
+ long randDelay = jitter(REFRESH_PERIOD_MINUTES / 4, REFRESH_PERIOD_MINUTES);
+
+ ScheduledThreadPoolExecutor executor = ThreadPools.getServerThreadPools()
+ .createScheduledExecutorService(4, "config-refresh", false);
Review Comment:
Only have one task to repeat, so we only need 1 thread in the pool.
```suggestion
.createScheduledExecutorService(1, "config-refresh", false);
```
##########
server/base/src/main/java/org/apache/accumulo/server/conf/ServerConfigurationFactory.java:
##########
@@ -140,4 +158,114 @@ public void connectionEvent() {
// no-op. changes handled by prop store impl
}
}
+
+ private class ConfigRefreshRunner {
+ private static final long MIN_JITTER_DELAY = 1;
+
+ private static final long MAX_JITTER_DELAY = 23;
+ private final ScheduledFuture<?> refreshTaskFuture;
+
+ ConfigRefreshRunner() {
+
+ Runnable refreshTask = this::verifySnapshotVersions;
+ // staggering the initial delay prevents synchronization of Accumulo servers communicating
+ // with ZooKeeper for the sync process.
+ long randDelay = jitter(REFRESH_PERIOD_MINUTES / 4, REFRESH_PERIOD_MINUTES);
+
+ ScheduledThreadPoolExecutor executor = ThreadPools.getServerThreadPools()
+ .createScheduledExecutorService(4, "config-refresh", false);
+ refreshTaskFuture =
+ executor.scheduleWithFixedDelay(refreshTask, randDelay, REFRESH_PERIOD_MINUTES, MINUTES);
+ }
+
+ /**
+ * Check that the stored version in ZooKeeper matches the version held in the local snapshot.
+ * When a mismatch is detected, a change event is sent to the prop store which will cause a
+ * re-load. If the Zookeeper node has been deleted, the local cache entries are removed.
+ * <p>
+ * This method is designed to be called as a scheduled task, so it does not propagate exceptions
+ * other than interrupted Exceptions so the scheduled tasks will continue to run.
+ */
+
+ private synchronized void verifySnapshotVersions() {
+ long refreshStart = System.nanoTime();
+ int keyCount = 0;
+ int keyChangedCount = 0;
+
+ PropStore propStore = context.getPropStore();
+ keyCount++;
+
+ // rely on store to propagate change event if different
+ propStore.validateDataVersion(SystemPropKey.of(context),
+ ((ZooBasedConfiguration) getSystemConfiguration()).getDataVersion());
+ // small yield - spread out ZooKeeper calls
+ jitterDelay();
+
+ for (Map.Entry<NamespaceId,NamespaceConfiguration> entry : namespaceConfigs.entrySet()) {
+ keyCount++;
+ PropStoreKey<?> propKey = NamespacePropKey.of(context, entry.getKey());
+ if (!propStore.validateDataVersion(propKey, entry.getValue().getDataVersion())) {
+ keyChangedCount++;
+ namespaceConfigs.remove(entry.getKey());
+ }
+ // small yield - spread out ZooKeeper calls between namespace config checks
+ jitterDelay();
+ }
+
+ for (Map.Entry<TableId,TableConfiguration> entry : tableConfigs.entrySet()) {
+ keyCount++;
+ TableId tid = entry.getKey();
+ PropStoreKey<?> propKey = TablePropKey.of(context, tid);
+ if (!propStore.validateDataVersion(propKey, entry.getValue().getDataVersion())) {
+ keyChangedCount++;
+ tableConfigs.remove(tid);
+ tableParentConfigs.remove(tid);
+ log.debug("data version sync: difference found. forcing configuration update for {}}",
+ propKey);
+ }
+ // small yield - spread out ZooKeeper calls between table config checks
+ jitterDelay();
+ }
+
+ log.debug("data version sync: Total runtime {} ms for {} entries, changes detected: {}",
+ NANOSECONDS.toMillis(System.nanoTime() - refreshStart), keyCount, keyChangedCount);
+ }
+
+ /**
+ * Generate a small random integer for jitter between [min,max).
+ *
+ * @param min
+ * minimum number (inclusive)
+ *
+ * @param max
+ * maximum number (exclusive)
+ *
+ * @return random number between min and MAJ_JITTER_DELAY
+ */
+ @SuppressFBWarnings(value = "PREDICTABLE_RANDOM",
+ justification = "random number not used in secure context")
+ private long jitter(final long min, final long max) {
+ return ThreadLocalRandom.current().nextLong(min, max);
+ }
Review Comment:
There's a lot of lines for the javadoc and boilerplate method, for a one-line method body that's only called in one place. I think it could be inline'd, and the javadoc isn't necessary, since the function is obvious once all the surrounding noise is removed.
##########
server/base/src/main/java/org/apache/accumulo/server/conf/store/impl/ZooPropStore.java:
##########
@@ -117,7 +121,12 @@ private ZooPropStore(final InstanceId instanceId, final ZooReaderWriter zrw) {
throw new IllegalStateException("Instance may not have been initialized, root node: " + path
+ " does not exist in ZooKeeper");
}
- } catch (InterruptedException | KeeperException ex) {
+ } catch (InterruptedException ex) {
+ Thread.currentThread().interrupt();
+ ;
Review Comment:
extra semicolon
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org