Make timeout behavior consistent to document by jihoonson · Pull Request #4134 · apache/druid

jihoonson · 2017-03-30T10:00:49Z

Reported here.

The default timeout is 0 which was previously maxIdleTime.
0 timeout means no timeout which previously means no wait.

This change is

jihoonson · 2017-03-30T16:00:16Z

Travis failure seems not related to this patch. All tests passed in my local machine.

leventov · 2017-03-31T19:18:40Z

-                                timeoutAt - (System.currentTimeMillis() - start)
-                            );
+                            if (hasTimeout) {
+                              responseContext.put(


Thanks for letting me know. This behavior looks weird, so I'd like to fix it in this pr if it doesn't require so large change. Since I don't know exactly how the responseContext is supposed to be used, let me make sure what the correct solution is. Is it ok to just move CTX_TIMEOUT_AT to QueryContext instead of ResponseContext?

Maybe it's not that easy to fix this, because CTX_TIMEOUT_AT and CTX_COUNT are used in some non-obvious way in ScanQueryEngine

Ok, then I'll look at after this pr.

leventov · 2017-03-31T19:21:16Z

-        final Number queryTimeout = query.getContextValue(QueryContextKeys.TIMEOUT, null);
-        final long timeoutAt = (queryTimeout == null || queryTimeout.longValue() == 0L)
-                               ? JodaUtils.MAX_INSTANT : System.currentTimeMillis() + queryTimeout.longValue();
+        final long timeoutAt = System.currentTimeMillis() + QueryContexts.getTimeout(query);


Here timeoutAt is set to System.currentTimeMillis() if timeout is not specified, is that what you wanted to do?

timeoutAt is used only if timeout is specified (https://github.com/jihoonson/druid/blob/8b64ce6ec9e083632bce7bf9fcc365b1d857a41b/extensions-contrib/scan-query/src/main/java/io/druid/query/scan/ScanQueryEngine.java#L161-L182).

Please add a comment in code

leventov · 2017-03-31T19:21:42Z

-          Number timeout = query.getContextValue(QueryContextKeys.TIMEOUT);
-          if (timeout == null) {
+          final long timeout = QueryContexts.getTimeout(query);
+          if (timeout == 0) {


QueryContexts.DEFAULT_TIMEOUT? Or add a method QueryContext.isNoTimeout(timeout)

leventov · 2017-03-31T19:22:53Z

              return new MergeIterable<>(
                  ordering.nullsFirst(),
-                  timeout == null ?
+                  timeout == 0 ?


leventov · 2017-03-31T19:23:51Z

-      final Number timeout = query.getContextValue(QueryContextKeys.TIMEOUT, (Number) null);
-      if (timeout == null) {
+      final long timeout = QueryContexts.getTimeout(query);
+      if (timeout == 0) {


Same as above

leventov · 2017-03-31T19:26:29Z

  public Sequence<T> run(final Query<T> query, Map<String, Object> responseContext)
  {
-    if (BaseQuery.getContextBySegment(query, false)) {
+    if (QueryContexts.isBySegment(query, false)) {


isBySegment is always called with false as the default argument, maybe remove the parameter

leventov · 2017-03-31T19:33:12Z

+                    throw new TimeoutException();
+                  }
+                } else {
+                  mergeBufferHolder = mergeBufferPool.take(-1);


In the spirit of blocking APIs from JDK to have a method that takes a timeout and timeUnit, and a method without parameters for indefinite blocking, rather than encoding this by negative argument.

leventov · 2017-03-31T19:34:51Z

+        final long timeout = QueryContexts.getTimeout(query);
        final ResourceHolder<List<ByteBuffer>> mergeBufferHolders = mergeBufferPool.takeBatch(
-            requiredMergeBufferNum, timeout.longValue()
+            requiredMergeBufferNum, timeout


takeBatch() javadoc says that "negative means no timeout", here timeout is QueryContext.DEFAULT_TIMEOUT, that is 0.

leventov · 2017-03-31T19:35:10Z

-                      final Number timeout = query.getContextValue(QueryContextKeys.TIMEOUT, (Number) null);
-                      return timeout == null ? future.get() : future.get(timeout.longValue(), TimeUnit.MILLISECONDS);
+                      final long timeout = QueryContexts.getTimeout(query);
+                      return timeout == 0 ? future.get() : future.get(timeout, TimeUnit.MILLISECONDS);


Same as above

leventov · 2017-03-31T19:36:25Z

-            )
-        );
+
+      if (QueryContexts.getTimeout(query) < 0) {


This check could be moved into QueryContexts.getTimeout()

leventov · 2017-04-03T21:32:42Z

   *
   * @return a resource, or null if the timeout was reached
   */
  public ReferenceCountingResourceHolder<T> take(final long timeout)


If you don't want to add a time unit parameter (which in my choice even if the method is always called with the same time unit argument, for readability on the caller side), please rename the parameter to "timeoutMs".

leventov · 2017-04-03T21:37:14Z

-            }
-          }
-      );
+      return wrapObject(timeout > 0 ? pollObject(timeout) : pollObject());


Since there is a dedicated no-timeout method, in this method I suggest to check if timeout is positive and throw an exception otherwise.

Added a check.

leventov · 2017-04-03T21:45:09Z

      queryWatcher.registerQuery(query, future);
      final long timeout = QueryContexts.getTimeout(query);
-      if (timeout == 0) {
+      if (QueryContexts.isNoTimeout(timeout)) {


I think it's clearer here:

if (QueryContext.hasTimeout(query)) { future.get(QueryContext.getTimeout(query), TimUnit.MILLISECONDS); } else { future.get(); }

This is also true for other places where isNoTimeout() is used, except the one in ChainedExecutionQueryRunner.

What do you think about this?

@jihoonson please comment on this

Ah, sorry I missed this comment. Yeah, it looks good. I changed including ChainedExecutionQueryRunner. It also looks simple and good.

leventov · 2017-04-03T21:48:14Z

-        final Number queryTimeout = query.getContextValue(QueryContextKeys.TIMEOUT, null);
-        final long timeoutAt = (queryTimeout == null || queryTimeout.longValue() == 0L)
-                               ? JodaUtils.MAX_INSTANT : System.currentTimeMillis() + queryTimeout.longValue();
+        final long timeoutAt = System.currentTimeMillis() + QueryContexts.getTimeout(query);


Please add a comment in code

leventov · 2017-04-03T21:53:30Z

-                                timeoutAt - (System.currentTimeMillis() - start)
-                            );
+                            if (hasTimeout) {
+                              responseContext.put(


Maybe it's not that easy to fix this, because CTX_TIMEOUT_AT and CTX_COUNT are used in some non-obvious way in ScanQueryEngine

jihoonson · 2017-04-04T00:48:24Z

@leventov, thanks. I've addressed your comments.

leventov · 2017-04-04T01:17:10Z

   * @return a resource, or null if the timeout was reached
   */
-  public ReferenceCountingResourceHolder<T> take(final long timeout)
+  public ReferenceCountingResourceHolder<T> take(final long timeoutMs)


It still has non-obvious undocumented corner case behaviour if timeoutMs=0, than it waits indefinitely, rather than not waiting at all.

I don't understand this comment.. If timeoutMs = 0, then take() immediately returns an object or null without waiting at all. Actually, negative timeoutMss have the same meaning.

Yes, my oversight

leventov · 2017-04-04T01:18:36Z

      queryWatcher.registerQuery(query, future);
      final long timeout = QueryContexts.getTimeout(query);
-      if (timeout == 0) {
+      if (QueryContexts.isNoTimeout(timeout)) {


What do you think about this?

leventov

Now isNoTimeout() seems to be unused

jihoonson · 2017-04-06T07:14:15Z

@leventov I first thought that it could be used in the future, so kept it. But, yeah, it's simple and can add again if necessary. I removed now.

…imeout

jihoonson · 2017-04-18T01:01:17Z

Would anyone please review this patch?

gianm · 2017-04-18T01:43:11Z

Taking a look.

gianm

LGTM other than comment about how the default query timeout config is dealt with.

gianm · 2017-04-18T01:49:43Z

        queryId = UUID.randomUUID().toString();
        query = query.withId(queryId);
      }
-      if (query.getContextValue(QueryContextKeys.TIMEOUT) == null) {


Instead of changing the default behavior, how about keeping the default behavior the same, but fixing the docs and making it more configurable? So I'm suggesting:

Add a druid.server.http.defaultQueryTimeout property and document that in configuration/broker.md and configuration/historical.md. Use that to set the default timeout here, and default that property to PT5M.

Update query-context.md to reflect that the default isn't 0, it's the value of druid.server.http.defaultQueryTimeout.

Sounds good. I changed the default query timeout to be configurable.

Sorry @jihoonson, I wasn't clear. For backwards compatibility reasons the query context parameter should stay milliseconds. Since druid.server.http.defaultQueryTimeout is new it could either be milliseconds or a period.

Right. I changed both of them to millis.

gianm

👍

leventov · 2017-04-19T14:23:44Z


  Query<T> withDataSource(DataSource dataSource);
+
+  Query<T> withDefaultTimeout(long defaultTimeout);


The problem with #4131 was that it breaks compatibility with extension queries, doesn't this PR break it already? @gianm @cheddar

Yes, you're right. I missed this and we need to fix it.

Patch in #4185.

Make timeout behavior consistent to document

8b64ce6

fjy added the Bug label Mar 30, 2017

fjy added this to the 0.10.1 milestone Mar 30, 2017

leventov reviewed Mar 31, 2017

View reviewed changes

jihoonson added 2 commits April 1, 2017 12:02

Refactoring BlockingPool and add more methods to QueryContexts

880d848

remove unused imports

7196bec

leventov reviewed Apr 3, 2017

View reviewed changes

Addressed comments

a899554

leventov reviewed Apr 4, 2017

View reviewed changes

arpankhagram mentioned this pull request Apr 4, 2017

groupBy v2: Multi-level merging of spill files #3833

Closed

Address comments

8c2d10f

leventov reviewed Apr 6, 2017

View reviewed changes

remove unused method

8b3c009

leventov approved these changes Apr 6, 2017

View reviewed changes

Merge branch 'master' of https://github.com/druid-io/druid into fix-t…

b9fe1ef

…imeout

gianm reviewed Apr 18, 2017

View reviewed changes

jihoonson added 3 commits April 18, 2017 21:25

Make default query timeout configurable

c9ce123

Fix test failure

a510acf

Change timeout from period to millis

526f708

gianm approved these changes Apr 19, 2017

View reviewed changes

gianm merged commit 5b69f2e into apache:master Apr 19, 2017

leventov reviewed Apr 19, 2017

View reviewed changes

gianm mentioned this pull request Apr 19, 2017

Restore backwards compatibility of Query. #4185

Merged

jihoonson deleted the fix-timeout branch December 5, 2018 22:49


		Query<T> withDataSource(DataSource dataSource);

		Query<T> withDefaultTimeout(long defaultTimeout);

Conversation

jihoonson commented Mar 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jihoonson commented Mar 30, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jihoonson commented Apr 4, 2017

Uh oh!

leventov Apr 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jihoonson commented Mar 30, 2017 •

edited

Loading

leventov Apr 4, 2017 •

edited

Loading

leventov left a comment •

edited

Loading