You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by "kfaraz (via GitHub)" <gi...@apache.org> on 2023/06/22 03:46:09 UTC
[GitHub] [druid] kfaraz opened a new pull request, #14468: Fix initialization bug in HttpServerInventoryView and improve monitoring
kfaraz opened a new pull request, #14468:
URL: https://github.com/apache/druid/pull/14468
### Description
The logs from `HttpServerInventoryView` and the underlying `ChangeRequestHttpSyncer` are often too cryptic and the metrics are insufficient.
### Bug
Description: If a server is removed during `HttpServerInventoryView.serverInventoryInitialized`, the initialization gets stuck as this server is never synced. The method eventually times out (default 250s).
Fix: Mark a server as stopped if it is removed. `serverInventoryInitialized` only waits for non-stopped servers to sync.
### Changes
- Add a new single-threaded monitoring executor in `HttpServerInventoryView` to emit metrics and reset unhealthy servers. Before this PR, server reset used to happen on the same executor which is used for syncing with servers.
- Add new metrics
- `cluster/serverview/healthy`: tracks if the server view is syncing properly with a server
- `cluster/serverview/unstable/time`: tracks the time in millis for which sync with a server has been unstable
- Improve logging in `HttpServerInventoryView` and `ChangeRequestHttpSyncer`
- Refactor for readability
- Add some static utility methods in `DateTimes` to compute elapsed time.
### Release note
<hr>
This PR has:
- [ ] been self-reviewed.
- [ ] using the [concurrency checklist](https://github.com/apache/druid/blob/master/dev/code-review/concurrency.md) (Remove this item if the PR doesn't have any relation to concurrency.)
- [ ] added documentation for new or modified features or behaviors.
- [ ] a release note entry in the PR description.
- [ ] added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
- [ ] added or updated version, license, or notice information in [licenses.yaml](https://github.com/apache/druid/blob/master/dev/license.md)
- [ ] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
- [ ] added unit tests or modified existing tests to cover new code paths, ensuring the threshold for [code coverage](https://github.com/apache/druid/blob/master/dev/code-review/code-coverage.md) is met.
- [ ] added integration tests.
- [ ] been tested in a test Druid cluster.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Fix initialization bug in HttpServerInventoryView and improve monitoring (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on code in PR #14468:
URL: https://github.com/apache/druid/pull/14468#discussion_r1247510317
##########
server/src/main/java/org/apache/druid/server/coordination/ChangeRequestHttpSyncer.java:
##########
@@ -157,56 +176,85 @@ public void stop()
}
}
- /** Wait for first fetch of segment listing from server. */
- public boolean awaitInitialization(long timeout, TimeUnit timeUnit) throws InterruptedException
+ /**
+ * Waits for the first successful sync with this server up to .
+ */
+ public boolean awaitInitialization() throws InterruptedException
{
- return initializationLatch.await(timeout, timeUnit);
+ return initializationLatch.await(maxMillisToWaitForSync, TimeUnit.MILLISECONDS);
}
/**
- * This method returns the debugging information for printing, must not be used for any other purpose.
+ * Waits upto 1 milliseconds for the first successful sync with this server.
*/
- public Map<String, Object> getDebugInfo()
+ public boolean isInitialized() throws InterruptedException
{
- long currTime = System.currentTimeMillis();
+ return initializationLatch.await(1, TimeUnit.MILLISECONDS);
+ }
- Object notSuccessfullySyncedFor;
- if (lastSuccessfulSyncTime == 0) {
- notSuccessfullySyncedFor = "Never Successfully Synced";
- } else {
- notSuccessfullySyncedFor = (currTime - lastSuccessfulSyncTime) / 1000;
- }
+ /**
+ * Returns debugging information for printing, must not be used for any other purpose.
+ */
+ public Map<String, Object> getDebugInfo()
+ {
return ImmutableMap.of(
- "notSyncedForSecs", lastSyncTime == 0 ? "Never Synced" : (currTime - lastSyncTime) / 1000,
- "notSuccessfullySyncedFor", notSuccessfullySyncedFor,
- "consecutiveFailedAttemptCount", consecutiveFailedAttemptCount,
+ "millisSinceLastSync",
+ sinceLastSyncRequest.isRunning() ? DateTimes.millisElapsed(sinceLastSyncRequest) : "Never synced",
+ "millisSinceLastSuccess",
+ sinceLastSyncSuccess.isRunning() ? DateTimes.millisElapsed(sinceLastSyncSuccess) : "Never synced successfully",
+ "millisUnstableDuration",
+ sinceUnstable.isRunning() ? DateTimes.millisElapsed(sinceUnstable) : "Stable",
+ "numRecentFailures", numRecentFailures,
"syncScheduled", startStopLock.isStarted()
);
}
+ private boolean hasSyncedSuccessfullyOnce()
+ {
+ return initializationLatch.getCount() <= 0;
+ }
+
/**
- * Exposed for monitoring use to see if sync is working fine and not stopped due to any coding bugs. If this
- * ever returns false then caller of this method must create an alert and it should be looked into for any
- * bugs.
+ * Whether this syncer should be reset. This method returning true typically
+ * indicates a problem with the sync scheduler.
+ *
+ * @return true if the delay since the last sync request sent to the server
+ * has exceeded {@link #maxDelayBetweenSyncRequests}, false otherwise.
*/
- public boolean isOK()
+ public boolean needsReset()
{
- return (System.currentTimeMillis() - lastSyncTime) < MAX_RETRY_BACKOFF + 3 * serverHttpTimeout;
+ return DateTimes.hasElapsed(maxDelayBetweenSyncRequests, sinceLastSyncRequest);
+ }
+
+ /**
+ * @return true if there have been no sync failures recently and the last
+ * successful sync was not more than {@link #maxMillisToWaitForSync} ago.
+ */
+ public boolean isSyncedSuccessfully()
+ {
+ final Duration timeoutDuration = Duration.millis(maxMillisToWaitForSync);
+ if (numRecentFailures > 0) {
+ return false;
+ } else if (hasSyncedSuccessfullyOnce()) {
+ return DateTimes.hasNotElapsed(timeoutDuration, sinceLastSyncSuccess);
+ } else {
+ return DateTimes.hasNotElapsed(timeoutDuration, sinceSyncerStart);
Review Comment:
Yeah, good point. I will rename the method.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] [WIP] Fix initialization bug in HttpServerInventoryView and improve monitoring (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on PR #14468:
URL: https://github.com/apache/druid/pull/14468#issuecomment-1623142308
Closing this as #14517 is already merged.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] kfaraz commented on a diff in pull request #14468: Fix initialization bug in HttpServerInventoryView and improve monitoring
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on code in PR #14468:
URL: https://github.com/apache/druid/pull/14468#discussion_r1245247397
##########
server/src/test/java/org/apache/druid/server/coordination/ChangeRequestHttpSyncerTest.java:
##########
@@ -22,106 +22,45 @@
import com.fasterxml.jackson.core.type.TypeReference;
import com.fasterxml.jackson.databind.ObjectMapper;
import com.google.common.collect.ImmutableList;
-import com.google.common.util.concurrent.Futures;
-import com.google.common.util.concurrent.ListenableFuture;
-import org.apache.druid.java.util.common.RE;
+import org.apache.druid.client.TestSegmentChangeRequestHttpClient;
+import org.apache.druid.java.util.common.ISE;
import org.apache.druid.java.util.common.concurrent.Execs;
-import org.apache.druid.java.util.http.client.HttpClient;
-import org.apache.druid.java.util.http.client.Request;
-import org.apache.druid.java.util.http.client.response.HttpResponseHandler;
import org.apache.druid.segment.TestHelper;
import org.easymock.EasyMock;
-import org.jboss.netty.buffer.ChannelBuffers;
-import org.jboss.netty.handler.codec.http.DefaultHttpResponse;
-import org.jboss.netty.handler.codec.http.HttpResponse;
-import org.jboss.netty.handler.codec.http.HttpResponseStatus;
-import org.jboss.netty.handler.codec.http.HttpVersion;
import org.joda.time.Duration;
import org.junit.Test;
-import java.io.ByteArrayInputStream;
import java.net.URL;
-import java.util.List;
-import java.util.concurrent.BlockingQueue;
-import java.util.concurrent.LinkedBlockingQueue;
-import java.util.concurrent.atomic.AtomicInteger;
+import java.util.Arrays;
/**
+ *
Review Comment:
Yeah, there are several tests which seem to have these.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Fix initialization bug in HttpServerInventoryView and improve monitoring (druid)
Posted by "findingrish (via GitHub)" <gi...@apache.org>.
findingrish commented on code in PR #14468:
URL: https://github.com/apache/druid/pull/14468#discussion_r1246335012
##########
server/src/main/java/org/apache/druid/server/coordination/ChangeRequestHttpSyncer.java:
##########
@@ -157,56 +176,85 @@ public void stop()
}
}
- /** Wait for first fetch of segment listing from server. */
- public boolean awaitInitialization(long timeout, TimeUnit timeUnit) throws InterruptedException
+ /**
+ * Waits for the first successful sync with this server up to .
Review Comment:
nit: incomplete comment?
##########
server/src/main/java/org/apache/druid/client/HttpServerInventoryView.java:
##########
@@ -300,72 +292,59 @@ private void runSegmentCallbacks(
{
for (final Map.Entry<SegmentCallback, Executor> entry : segmentCallbacks.entrySet()) {
entry.getValue().execute(
- new Runnable()
- {
- @Override
- public void run()
- {
- if (CallbackAction.UNREGISTER == fn.apply(entry.getKey())) {
- segmentCallbacks.remove(entry.getKey());
- if (segmentPredicates.remove(entry.getKey()) != null) {
- finalPredicate = Predicates.or(
- defaultFilter,
- Predicates.or(segmentPredicates.values())
- );
- }
+ () -> {
+ if (CallbackAction.UNREGISTER == fn.apply(entry.getKey())) {
+ segmentCallbacks.remove(entry.getKey());
+ if (segmentPredicates.remove(entry.getKey()) != null) {
+ updateFinalPredicate();
}
}
}
);
}
}
- private void runServerCallbacks(final DruidServer server)
+ private void runServerRemovedCallbacks(final DruidServer server)
{
for (final Map.Entry<ServerRemovedCallback, Executor> entry : serverCallbacks.entrySet()) {
entry.getValue().execute(
- new Runnable()
- {
- @Override
- public void run()
- {
- if (CallbackAction.UNREGISTER == entry.getKey().serverRemoved(server)) {
- serverCallbacks.remove(entry.getKey());
- }
+ () -> {
+ if (CallbackAction.UNREGISTER == entry.getKey().serverRemoved(server)) {
+ serverCallbacks.remove(entry.getKey());
}
}
);
}
}
- //best effort wait for first segment listing fetch from all servers and then call
- //segmentViewInitialized on all registered segment callbacks.
+ /**
+ * Waits until the sync wait timeout for all servers to be synced at least once.
+ * Finally calls {@link SegmentCallback#segmentViewInitialized()} regardless of
+ * whether all servers synced successfully or not.
+ */
private void serverInventoryInitialized()
{
- long start = System.currentTimeMillis();
- long serverSyncWaitTimeout = config.getServerTimeout() + 2 * ChangeRequestHttpSyncer.HTTP_TIMEOUT_EXTRA_MS;
+ final Stopwatch stopwatch = Stopwatch.createUnstarted();
+ final Duration syncWaitTimeout = Duration.millis(
+ config.getServerTimeout() + 2 * ChangeRequestHttpSyncer.MIN_READ_TIMEOUT_MILLIS
+ );
- List<DruidServerHolder> uninitializedServers = new ArrayList<>();
- for (DruidServerHolder server : servers.values()) {
- if (!server.isSyncedSuccessfullyAtleastOnce()) {
- uninitializedServers.add(server);
+ final List<DruidServerHolder> uninitializedServers = new ArrayList<>(servers.values());
+ while (DateTimes.hasNotElapsed(syncWaitTimeout, stopwatch)) {
+ uninitializedServers.removeIf(
+ serverHolder -> serverHolder.isSyncedSuccessfullyAtleastOnce()
+ || serverHolder.isStopped()
+ );
+ if (uninitializedServers.isEmpty()) {
Review Comment:
nit: For clarity could we club this with the loop condition?
##########
server/src/main/java/org/apache/druid/server/coordination/ChangeRequestHttpSyncer.java:
##########
@@ -157,56 +176,85 @@ public void stop()
}
}
- /** Wait for first fetch of segment listing from server. */
- public boolean awaitInitialization(long timeout, TimeUnit timeUnit) throws InterruptedException
+ /**
+ * Waits for the first successful sync with this server up to .
+ */
+ public boolean awaitInitialization() throws InterruptedException
{
- return initializationLatch.await(timeout, timeUnit);
+ return initializationLatch.await(maxMillisToWaitForSync, TimeUnit.MILLISECONDS);
}
/**
- * This method returns the debugging information for printing, must not be used for any other purpose.
+ * Waits upto 1 milliseconds for the first successful sync with this server.
*/
- public Map<String, Object> getDebugInfo()
+ public boolean isInitialized() throws InterruptedException
{
- long currTime = System.currentTimeMillis();
+ return initializationLatch.await(1, TimeUnit.MILLISECONDS);
+ }
- Object notSuccessfullySyncedFor;
- if (lastSuccessfulSyncTime == 0) {
- notSuccessfullySyncedFor = "Never Successfully Synced";
- } else {
- notSuccessfullySyncedFor = (currTime - lastSuccessfulSyncTime) / 1000;
- }
+ /**
+ * Returns debugging information for printing, must not be used for any other purpose.
+ */
+ public Map<String, Object> getDebugInfo()
+ {
return ImmutableMap.of(
- "notSyncedForSecs", lastSyncTime == 0 ? "Never Synced" : (currTime - lastSyncTime) / 1000,
- "notSuccessfullySyncedFor", notSuccessfullySyncedFor,
- "consecutiveFailedAttemptCount", consecutiveFailedAttemptCount,
+ "millisSinceLastSync",
+ sinceLastSyncRequest.isRunning() ? DateTimes.millisElapsed(sinceLastSyncRequest) : "Never synced",
+ "millisSinceLastSuccess",
+ sinceLastSyncSuccess.isRunning() ? DateTimes.millisElapsed(sinceLastSyncSuccess) : "Never synced successfully",
+ "millisUnstableDuration",
+ sinceUnstable.isRunning() ? DateTimes.millisElapsed(sinceUnstable) : "Stable",
+ "numRecentFailures", numRecentFailures,
"syncScheduled", startStopLock.isStarted()
);
}
+ private boolean hasSyncedSuccessfullyOnce()
+ {
+ return initializationLatch.getCount() <= 0;
+ }
+
/**
- * Exposed for monitoring use to see if sync is working fine and not stopped due to any coding bugs. If this
- * ever returns false then caller of this method must create an alert and it should be looked into for any
- * bugs.
+ * Whether this syncer should be reset. This method returning true typically
+ * indicates a problem with the sync scheduler.
+ *
+ * @return true if the delay since the last sync request sent to the server
+ * has exceeded {@link #maxDelayBetweenSyncRequests}, false otherwise.
*/
- public boolean isOK()
+ public boolean needsReset()
{
- return (System.currentTimeMillis() - lastSyncTime) < MAX_RETRY_BACKOFF + 3 * serverHttpTimeout;
+ return DateTimes.hasElapsed(maxDelayBetweenSyncRequests, sinceLastSyncRequest);
+ }
+
+ /**
+ * @return true if there have been no sync failures recently and the last
+ * successful sync was not more than {@link #maxMillisToWaitForSync} ago.
+ */
+ public boolean isSyncedSuccessfully()
+ {
+ final Duration timeoutDuration = Duration.millis(maxMillisToWaitForSync);
+ if (numRecentFailures > 0) {
+ return false;
+ } else if (hasSyncedSuccessfullyOnce()) {
+ return DateTimes.hasNotElapsed(timeoutDuration, sinceLastSyncSuccess);
+ } else {
+ return DateTimes.hasNotElapsed(timeoutDuration, sinceSyncerStart);
Review Comment:
Shouldn't we return false in this case?
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Fix initialization bug in HttpServerInventoryView and improve monitoring (druid)
Posted by "findingrish (via GitHub)" <gi...@apache.org>.
findingrish commented on code in PR #14468:
URL: https://github.com/apache/druid/pull/14468#discussion_r1247448545
##########
server/src/main/java/org/apache/druid/server/coordination/ChangeRequestHttpSyncer.java:
##########
@@ -157,56 +176,85 @@ public void stop()
}
}
- /** Wait for first fetch of segment listing from server. */
- public boolean awaitInitialization(long timeout, TimeUnit timeUnit) throws InterruptedException
+ /**
+ * Waits for the first successful sync with this server up to .
+ */
+ public boolean awaitInitialization() throws InterruptedException
{
- return initializationLatch.await(timeout, timeUnit);
+ return initializationLatch.await(maxMillisToWaitForSync, TimeUnit.MILLISECONDS);
}
/**
- * This method returns the debugging information for printing, must not be used for any other purpose.
+ * Waits upto 1 milliseconds for the first successful sync with this server.
*/
- public Map<String, Object> getDebugInfo()
+ public boolean isInitialized() throws InterruptedException
{
- long currTime = System.currentTimeMillis();
+ return initializationLatch.await(1, TimeUnit.MILLISECONDS);
+ }
- Object notSuccessfullySyncedFor;
- if (lastSuccessfulSyncTime == 0) {
- notSuccessfullySyncedFor = "Never Successfully Synced";
- } else {
- notSuccessfullySyncedFor = (currTime - lastSuccessfulSyncTime) / 1000;
- }
+ /**
+ * Returns debugging information for printing, must not be used for any other purpose.
+ */
+ public Map<String, Object> getDebugInfo()
+ {
return ImmutableMap.of(
- "notSyncedForSecs", lastSyncTime == 0 ? "Never Synced" : (currTime - lastSyncTime) / 1000,
- "notSuccessfullySyncedFor", notSuccessfullySyncedFor,
- "consecutiveFailedAttemptCount", consecutiveFailedAttemptCount,
+ "millisSinceLastSync",
+ sinceLastSyncRequest.isRunning() ? DateTimes.millisElapsed(sinceLastSyncRequest) : "Never synced",
+ "millisSinceLastSuccess",
+ sinceLastSyncSuccess.isRunning() ? DateTimes.millisElapsed(sinceLastSyncSuccess) : "Never synced successfully",
+ "millisUnstableDuration",
+ sinceUnstable.isRunning() ? DateTimes.millisElapsed(sinceUnstable) : "Stable",
+ "numRecentFailures", numRecentFailures,
"syncScheduled", startStopLock.isStarted()
);
}
+ private boolean hasSyncedSuccessfullyOnce()
+ {
+ return initializationLatch.getCount() <= 0;
+ }
+
/**
- * Exposed for monitoring use to see if sync is working fine and not stopped due to any coding bugs. If this
- * ever returns false then caller of this method must create an alert and it should be looked into for any
- * bugs.
+ * Whether this syncer should be reset. This method returning true typically
+ * indicates a problem with the sync scheduler.
+ *
+ * @return true if the delay since the last sync request sent to the server
+ * has exceeded {@link #maxDelayBetweenSyncRequests}, false otherwise.
*/
- public boolean isOK()
+ public boolean needsReset()
{
- return (System.currentTimeMillis() - lastSyncTime) < MAX_RETRY_BACKOFF + 3 * serverHttpTimeout;
+ return DateTimes.hasElapsed(maxDelayBetweenSyncRequests, sinceLastSyncRequest);
+ }
+
+ /**
+ * @return true if there have been no sync failures recently and the last
+ * successful sync was not more than {@link #maxMillisToWaitForSync} ago.
+ */
+ public boolean isSyncedSuccessfully()
+ {
+ final Duration timeoutDuration = Duration.millis(maxMillisToWaitForSync);
+ if (numRecentFailures > 0) {
+ return false;
+ } else if (hasSyncedSuccessfullyOnce()) {
+ return DateTimes.hasNotElapsed(timeoutDuration, sinceLastSyncSuccess);
+ } else {
+ return DateTimes.hasNotElapsed(timeoutDuration, sinceSyncerStart);
Review Comment:
Okay, got confused by the method name, which seems to be suggesting that we have synced successfully at least once.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] kfaraz commented on a diff in pull request #14468: Fix initialization bug in HttpServerInventoryView and improve monitoring
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on code in PR #14468:
URL: https://github.com/apache/druid/pull/14468#discussion_r1244579809
##########
processing/src/test/java/org/apache/druid/java/util/common/DateTimesTest.java:
##########
@@ -123,4 +126,28 @@ public void testCanCompareAsString()
DateTimes.of("2000").withZone(DateTimes.inferTzFromString("America/Los_Angeles")))
);
}
+
+ @Test
+ public void testMillisElapsed() throws Exception
+ {
+ Stopwatch stopwatch = Stopwatch.createStarted();
+ Thread.sleep(100);
Review Comment:
Yeah, good idea.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Fix initialization bug in HttpServerInventoryView and improve monitoring (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on code in PR #14468:
URL: https://github.com/apache/druid/pull/14468#discussion_r1247388197
##########
server/src/main/java/org/apache/druid/server/coordination/ChangeRequestHttpSyncer.java:
##########
@@ -157,56 +176,85 @@ public void stop()
}
}
- /** Wait for first fetch of segment listing from server. */
- public boolean awaitInitialization(long timeout, TimeUnit timeUnit) throws InterruptedException
+ /**
+ * Waits for the first successful sync with this server up to .
+ */
+ public boolean awaitInitialization() throws InterruptedException
{
- return initializationLatch.await(timeout, timeUnit);
+ return initializationLatch.await(maxMillisToWaitForSync, TimeUnit.MILLISECONDS);
}
/**
- * This method returns the debugging information for printing, must not be used for any other purpose.
+ * Waits upto 1 milliseconds for the first successful sync with this server.
*/
- public Map<String, Object> getDebugInfo()
+ public boolean isInitialized() throws InterruptedException
{
- long currTime = System.currentTimeMillis();
+ return initializationLatch.await(1, TimeUnit.MILLISECONDS);
+ }
- Object notSuccessfullySyncedFor;
- if (lastSuccessfulSyncTime == 0) {
- notSuccessfullySyncedFor = "Never Successfully Synced";
- } else {
- notSuccessfullySyncedFor = (currTime - lastSuccessfulSyncTime) / 1000;
- }
+ /**
+ * Returns debugging information for printing, must not be used for any other purpose.
+ */
+ public Map<String, Object> getDebugInfo()
+ {
return ImmutableMap.of(
- "notSyncedForSecs", lastSyncTime == 0 ? "Never Synced" : (currTime - lastSyncTime) / 1000,
- "notSuccessfullySyncedFor", notSuccessfullySyncedFor,
- "consecutiveFailedAttemptCount", consecutiveFailedAttemptCount,
+ "millisSinceLastSync",
+ sinceLastSyncRequest.isRunning() ? DateTimes.millisElapsed(sinceLastSyncRequest) : "Never synced",
+ "millisSinceLastSuccess",
+ sinceLastSyncSuccess.isRunning() ? DateTimes.millisElapsed(sinceLastSyncSuccess) : "Never synced successfully",
+ "millisUnstableDuration",
+ sinceUnstable.isRunning() ? DateTimes.millisElapsed(sinceUnstable) : "Stable",
+ "numRecentFailures", numRecentFailures,
"syncScheduled", startStopLock.isStarted()
);
}
+ private boolean hasSyncedSuccessfullyOnce()
+ {
+ return initializationLatch.getCount() <= 0;
+ }
+
/**
- * Exposed for monitoring use to see if sync is working fine and not stopped due to any coding bugs. If this
- * ever returns false then caller of this method must create an alert and it should be looked into for any
- * bugs.
+ * Whether this syncer should be reset. This method returning true typically
+ * indicates a problem with the sync scheduler.
+ *
+ * @return true if the delay since the last sync request sent to the server
+ * has exceeded {@link #maxDelayBetweenSyncRequests}, false otherwise.
*/
- public boolean isOK()
+ public boolean needsReset()
{
- return (System.currentTimeMillis() - lastSyncTime) < MAX_RETRY_BACKOFF + 3 * serverHttpTimeout;
+ return DateTimes.hasElapsed(maxDelayBetweenSyncRequests, sinceLastSyncRequest);
+ }
+
+ /**
+ * @return true if there have been no sync failures recently and the last
+ * successful sync was not more than {@link #maxMillisToWaitForSync} ago.
+ */
+ public boolean isSyncedSuccessfully()
+ {
+ final Duration timeoutDuration = Duration.millis(maxMillisToWaitForSync);
+ if (numRecentFailures > 0) {
+ return false;
+ } else if (hasSyncedSuccessfullyOnce()) {
+ return DateTimes.hasNotElapsed(timeoutDuration, sinceLastSyncSuccess);
+ } else {
+ return DateTimes.hasNotElapsed(timeoutDuration, sinceSyncerStart);
Review Comment:
No, this statement says that we should wait for the timeoutDuration after startup before we make up our mind of syncer health. Until then, we just assume that the syncer is healthy. Otherwise, we would have a lot of unhealthy metrics right at startup, when in fact the initial delay is expected.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] Fix initialization bug in HttpServerInventoryView and improve monitoring (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz commented on code in PR #14468:
URL: https://github.com/apache/druid/pull/14468#discussion_r1247388656
##########
server/src/main/java/org/apache/druid/client/HttpServerInventoryView.java:
##########
@@ -300,72 +292,59 @@ private void runSegmentCallbacks(
{
for (final Map.Entry<SegmentCallback, Executor> entry : segmentCallbacks.entrySet()) {
entry.getValue().execute(
- new Runnable()
- {
- @Override
- public void run()
- {
- if (CallbackAction.UNREGISTER == fn.apply(entry.getKey())) {
- segmentCallbacks.remove(entry.getKey());
- if (segmentPredicates.remove(entry.getKey()) != null) {
- finalPredicate = Predicates.or(
- defaultFilter,
- Predicates.or(segmentPredicates.values())
- );
- }
+ () -> {
+ if (CallbackAction.UNREGISTER == fn.apply(entry.getKey())) {
+ segmentCallbacks.remove(entry.getKey());
+ if (segmentPredicates.remove(entry.getKey()) != null) {
+ updateFinalPredicate();
}
}
}
);
}
}
- private void runServerCallbacks(final DruidServer server)
+ private void runServerRemovedCallbacks(final DruidServer server)
{
for (final Map.Entry<ServerRemovedCallback, Executor> entry : serverCallbacks.entrySet()) {
entry.getValue().execute(
- new Runnable()
- {
- @Override
- public void run()
- {
- if (CallbackAction.UNREGISTER == entry.getKey().serverRemoved(server)) {
- serverCallbacks.remove(entry.getKey());
- }
+ () -> {
+ if (CallbackAction.UNREGISTER == entry.getKey().serverRemoved(server)) {
+ serverCallbacks.remove(entry.getKey());
}
}
);
}
}
- //best effort wait for first segment listing fetch from all servers and then call
- //segmentViewInitialized on all registered segment callbacks.
+ /**
+ * Waits until the sync wait timeout for all servers to be synced at least once.
+ * Finally calls {@link SegmentCallback#segmentViewInitialized()} regardless of
+ * whether all servers synced successfully or not.
+ */
private void serverInventoryInitialized()
{
- long start = System.currentTimeMillis();
- long serverSyncWaitTimeout = config.getServerTimeout() + 2 * ChangeRequestHttpSyncer.HTTP_TIMEOUT_EXTRA_MS;
+ final Stopwatch stopwatch = Stopwatch.createUnstarted();
+ final Duration syncWaitTimeout = Duration.millis(
+ config.getServerTimeout() + 2 * ChangeRequestHttpSyncer.MIN_READ_TIMEOUT_MILLIS
+ );
- List<DruidServerHolder> uninitializedServers = new ArrayList<>();
- for (DruidServerHolder server : servers.values()) {
- if (!server.isSyncedSuccessfullyAtleastOnce()) {
- uninitializedServers.add(server);
+ final List<DruidServerHolder> uninitializedServers = new ArrayList<>(servers.values());
+ while (DateTimes.hasNotElapsed(syncWaitTimeout, stopwatch)) {
+ uninitializedServers.removeIf(
+ serverHolder -> serverHolder.isSyncedSuccessfullyAtleastOnce()
+ || serverHolder.isStopped()
+ );
+ if (uninitializedServers.isEmpty()) {
Review Comment:
The loop condition will be evaluated after the sleep of 5s. We don't want to wait if the list is already empty.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] [WIP] Fix initialization bug in HttpServerInventoryView and improve monitoring (druid)
Posted by "kfaraz (via GitHub)" <gi...@apache.org>.
kfaraz closed pull request #14468: [WIP] Fix initialization bug in HttpServerInventoryView and improve monitoring
URL: https://github.com/apache/druid/pull/14468
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] github-code-scanning[bot] commented on a diff in pull request #14468: Fix initialization bug in HttpServerInventoryView and improve monitoring
Posted by "github-code-scanning[bot] (via GitHub)" <gi...@apache.org>.
github-code-scanning[bot] commented on code in PR #14468:
URL: https://github.com/apache/druid/pull/14468#discussion_r1244787349
##########
server/src/test/java/org/apache/druid/client/TestSegmentChangeRequestHttpClient.java:
##########
@@ -0,0 +1,158 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.client;
+
+import com.fasterxml.jackson.core.JsonProcessingException;
+import com.fasterxml.jackson.core.type.TypeReference;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.google.common.util.concurrent.Futures;
+import com.google.common.util.concurrent.ListenableFuture;
+import org.apache.druid.error.DruidException;
+import org.apache.druid.error.ErrorResponse;
+import org.apache.druid.java.util.common.ISE;
+import org.apache.druid.java.util.common.RE;
+import org.apache.druid.java.util.http.client.HttpClient;
+import org.apache.druid.java.util.http.client.Request;
+import org.apache.druid.java.util.http.client.response.HttpResponseHandler;
+import org.apache.druid.segment.TestHelper;
+import org.jboss.netty.buffer.ChannelBuffers;
+import org.jboss.netty.handler.codec.http.DefaultHttpResponse;
+import org.jboss.netty.handler.codec.http.HttpResponse;
+import org.jboss.netty.handler.codec.http.HttpResponseStatus;
+import org.jboss.netty.handler.codec.http.HttpVersion;
+import org.joda.time.Duration;
+
+import java.io.ByteArrayInputStream;
+import java.util.concurrent.BlockingQueue;
+import java.util.concurrent.LinkedBlockingQueue;
+import java.util.function.Supplier;
+
+public class TestSegmentChangeRequestHttpClient implements HttpClient
+{
+ private final ObjectMapper MAPPER = TestHelper.makeJsonMapper();
+ private final BlockingQueue<ResultHolder<?>> results = new LinkedBlockingQueue<>();
+
+ private int requestCount;
+
+ public void failNextRequestOnClientWith(RuntimeException clientError)
+ {
+ results.add(new ResultHolder<>(null, null, clientError, null));
+ }
+
+ public void failNextRequestOnServerWith(DruidException serverError)
+ {
+ results.add(new ResultHolder<>(null, null, null, serverError));
+ }
+
+ public <T> void completeNextRequestWith(T result, TypeReference<T> typeReference)
+ {
+ results.add(new ResultHolder<>(() -> result, typeReference, null, null));
+ }
+
+ public boolean hasPendingResults()
+ {
+ return !results.isEmpty();
+ }
+
+ @Override
+ public <Intermediate, Final> ListenableFuture<Final> go(
+ Request request,
+ HttpResponseHandler<Intermediate, Final> httpResponseHandler
+ )
+ {
+ throw new UnsupportedOperationException("Not Implemented.");
+ }
+
+ @Override
+ public <Intermediate, Final> ListenableFuture<Final> go(
+ Request request,
+ HttpResponseHandler<Intermediate, Final> httpResponseHandler,
+ Duration duration
+ )
+ {
+ final int currentRequest = requestCount++;
Review Comment:
## User-controlled data in arithmetic expression
This arithmetic expression depends on a [user-provided value](1), potentially causing an overflow.
This arithmetic expression depends on a [user-provided value](2), potentially causing an overflow.
[Show more details](https://github.com/apache/druid/security/code-scanning/5197)
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] findingrish commented on a diff in pull request #14468: Fix initialization bug in HttpServerInventoryView and improve monitoring
Posted by "findingrish (via GitHub)" <gi...@apache.org>.
findingrish commented on code in PR #14468:
URL: https://github.com/apache/druid/pull/14468#discussion_r1244089296
##########
server/src/test/java/org/apache/druid/server/coordination/ChangeRequestHttpSyncerTest.java:
##########
@@ -22,106 +22,45 @@
import com.fasterxml.jackson.core.type.TypeReference;
import com.fasterxml.jackson.databind.ObjectMapper;
import com.google.common.collect.ImmutableList;
-import com.google.common.util.concurrent.Futures;
-import com.google.common.util.concurrent.ListenableFuture;
-import org.apache.druid.java.util.common.RE;
+import org.apache.druid.client.TestSegmentChangeRequestHttpClient;
+import org.apache.druid.java.util.common.ISE;
import org.apache.druid.java.util.common.concurrent.Execs;
-import org.apache.druid.java.util.http.client.HttpClient;
-import org.apache.druid.java.util.http.client.Request;
-import org.apache.druid.java.util.http.client.response.HttpResponseHandler;
import org.apache.druid.segment.TestHelper;
import org.easymock.EasyMock;
-import org.jboss.netty.buffer.ChannelBuffers;
-import org.jboss.netty.handler.codec.http.DefaultHttpResponse;
-import org.jboss.netty.handler.codec.http.HttpResponse;
-import org.jboss.netty.handler.codec.http.HttpResponseStatus;
-import org.jboss.netty.handler.codec.http.HttpVersion;
import org.joda.time.Duration;
import org.junit.Test;
-import java.io.ByteArrayInputStream;
import java.net.URL;
-import java.util.List;
-import java.util.concurrent.BlockingQueue;
-import java.util.concurrent.LinkedBlockingQueue;
-import java.util.concurrent.atomic.AtomicInteger;
+import java.util.Arrays;
/**
+ *
Review Comment:
nit: we can remove empty comment block
##########
processing/src/test/java/org/apache/druid/java/util/common/DateTimesTest.java:
##########
@@ -123,4 +126,28 @@ public void testCanCompareAsString()
DateTimes.of("2000").withZone(DateTimes.inferTzFromString("America/Los_Angeles")))
);
}
+
+ @Test
+ public void testMillisElapsed() throws Exception
+ {
+ Stopwatch stopwatch = Stopwatch.createStarted();
+ Thread.sleep(100);
+ stopwatch.stop();
+
+ Assert.assertEquals(
+ stopwatch.elapsed(TimeUnit.MILLISECONDS),
+ DateTimes.millisElapsed(stopwatch)
+ );
+ }
+
+ @Test
+ public void testHasElapsed() throws Exception
+ {
+ Stopwatch stopwatch = Stopwatch.createStarted();
+ Thread.sleep(100);
Review Comment:
same comment as above.
##########
processing/src/test/java/org/apache/druid/java/util/common/DateTimesTest.java:
##########
@@ -123,4 +126,28 @@ public void testCanCompareAsString()
DateTimes.of("2000").withZone(DateTimes.inferTzFromString("America/Los_Angeles")))
);
}
+
+ @Test
+ public void testMillisElapsed() throws Exception
+ {
+ Stopwatch stopwatch = Stopwatch.createStarted();
+ Thread.sleep(100);
Review Comment:
Instead of sleep, we can use a custom ticker to advance time artificially.
`FakeTicker` is a subclass which can be used to
```
FakeTicker fakeTicker = new FakeTicker();
Stopwatch stopwatch = Stopwatch.createStarted(fakeTicker);
fakeTicker.advance(100, TimeUnit.MILLIS);
```
##########
server/src/test/java/org/apache/druid/client/HttpServerInventoryViewTest.java:
##########
@@ -305,42 +311,28 @@ public ServerView.CallbackAction serverRemoved(DruidServer server)
serverRemovedCalled.await();
Assert.assertNull(httpServerInventoryView.getInventoryValue("host:8080"));
- EasyMock.verify(druidNodeDiscoveryProvider);
-
httpServerInventoryView.stop();
}
@Test(timeout = 60_000L)
public void testSyncMonitoring()
{
- ObjectMapper jsonMapper = TestHelper.makeJsonMapper();
-
- TestDruidNodeDiscovery druidNodeDiscovery = new TestDruidNodeDiscovery();
- DruidNodeDiscoveryProvider druidNodeDiscoveryProvider = EasyMock.createMock(DruidNodeDiscoveryProvider.class);
- EasyMock.expect(druidNodeDiscoveryProvider.getForService(DataNodeService.DISCOVERY_SERVICE_KEY))
- .andReturn(druidNodeDiscovery);
- EasyMock.replay(druidNodeDiscoveryProvider);
-
- TestHttpClient httpClient = new TestHttpClient(ImmutableList.of());
-
- HttpServerInventoryView httpServerInventoryView = new HttpServerInventoryView(
- jsonMapper,
- httpClient,
- druidNodeDiscoveryProvider,
- (pair) -> !pair.rhs.getDataSource().equals("non-loading-datasource"),
- new HttpServerInventoryViewConfig(null, null, null),
- "test"
- );
-
httpServerInventoryView.start();
httpServerInventoryView.serverAdded(makeServer("abc.com:8080"));
httpServerInventoryView.serverAdded(makeServer("xyz.com:8080"));
httpServerInventoryView.serverAdded(makeServer("lol.com:8080"));
Assert.assertEquals(3, httpServerInventoryView.getDebugInfo().size());
- httpServerInventoryView.syncMonitoring();
+ httpServerInventoryView.checkAndResetUnhealthyServers();
Assert.assertEquals(3, httpServerInventoryView.getDebugInfo().size());
}
+ @Test
+ @Ignore
+ public void testSyncStatusMetricsAreEmitted()
Review Comment:
should this be removed?
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org