Always return a response from query execution. #6596

amrishlal · 2021-02-19T20:55:47Z

Description

Server does not return a response for some of the queries that fail. As a result broker will wait until it times out. This causes un-necessary latency for queries on the Broker side. One example of such a query is:

select playerID from baseballStats where playerID in ("aardsda01")

This PR fixes the issue by always returning a response back to the Broker irrespective of whether query succeeds or fails. Note that UNCAUGHT_EXCEPTIONS and REQUEST_FETCH_EXCEPTION metrics are no longer being emitted.

Upgrade Notes

Does this PR prevent a zero down-time upgrade? (Assume upgrade order: Controller, Broker, Server, Minion)

Yes (Please label as backward-incompat, and complete the section below on Release Notes)

Does this PR fix a zero-downtime upgrade introduced earlier?

Yes (Please label this as backward-incompat, and complete the section below on Release Notes)

Does this PR otherwise need attention when creating release notes? Things to consider:

New configuration options
Deprecation of configurations
Signature changes to public methods/interfaces
New plugins added or old plugins removed

Yes (Please label this PR as release-notes and complete the section on Release Notes)

Release Notes

If you have tagged this as either backward-incompat or release-notes,
you MUST add text here that you would like to see appear in release notes of the
next release.

If you have a series of commits adding or enabling a feature, then
add this section only in final commit that marks the feature completed.
Refer to earlier release notes to see examples of text

Documentation

If you have introduced a new feature or configuration, please add it to the documentation as well.
See https://docs.pinot.apache.org/developers/developers-and-contributors/update-document

codecov-io · 2021-02-21T07:56:17Z

Codecov Report

Merging #6596 (ebffbb0) into master (1beaab5) will decrease coverage by 0.91%.
The diff coverage is 62.57%.

@@            Coverage Diff             @@
##           master    #6596      +/-   ##
==========================================
- Coverage   66.44%   65.53%   -0.92%     
==========================================
  Files        1075     1352     +277     
  Lines       54773    66558   +11785     
  Branches     8168     9697    +1529     
==========================================
+ Hits        36396    43621    +7225     
- Misses      15700    19820    +4120     
- Partials     2677     3117     +440

Flag	Coverage Δ
unittests	`65.53% <62.57%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...e/pinot/broker/api/resources/PinotBrokerDebug.java	`0.00% <0.00%> (-79.32%)`	⬇️
...pinot/broker/api/resources/PinotClientRequest.java	`0.00% <0.00%> (-27.28%)`	⬇️
...ot/broker/broker/AllowAllAccessControlFactory.java	`71.42% <ø> (-28.58%)`	⬇️
.../helix/BrokerUserDefinedMessageHandlerFactory.java	`33.96% <0.00%> (-32.71%)`	⬇️
...ker/routing/instanceselector/InstanceSelector.java	`100.00% <ø> (ø)`
...ava/org/apache/pinot/client/AbstractResultSet.java	`66.66% <ø> (+9.52%)`	⬆️
...n/java/org/apache/pinot/client/BrokerResponse.java	`100.00% <ø> (ø)`
.../main/java/org/apache/pinot/client/Connection.java	`35.55% <ø> (-13.29%)`	⬇️
...org/apache/pinot/client/DynamicBrokerSelector.java	`82.85% <ø> (+10.12%)`	⬆️
...n/java/org/apache/pinot/client/ExecutionStats.java	`68.88% <ø> (ø)`
... and 1215 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update be8044c...86aee9a. Read the comment docs.

pinot-core/src/main/java/org/apache/pinot/core/query/scheduler/QueryScheduler.java

mayankshriv · 2021-02-21T18:24:42Z

pinot-core/src/main/java/org/apache/pinot/core/transport/InstanceRequestHandler.java


    InstanceRequest instanceRequest = new InstanceRequest();
+    ServerQueryRequest queryRequest;
+    byte[] requestBytes = new byte[requestSize];


Why not put all non-declaration statements inside the try block, in the rare event an of these fail, the code will be able to handle it.

Refactored the code a bit more to do this.

mayankshriv · 2021-02-21T18:29:11Z

pinot-core/src/main/java/org/apache/pinot/core/transport/InstanceRequestHandler.java

      }
    }, MoreExecutors.directExecutor());
  }

  @Override
  public void exceptionCaught(ChannelHandlerContext ctx, Throwable cause) {
-    LOGGER.error("Caught exception while fetching instance request", cause);
-    _serverMetrics.addMeteredGlobalValue(ServerMeter.REQUEST_FETCH_EXCEPTIONS, 1);
+    // Since we do not know the requestId of the original request here, there is no way for Broker to know which query


Can we use ChannelHandlerContext for storing that, or is that a read-only object?

From what I can see in the javadoc, ChannelHandlerContext doesn't provide this functionality.

@amrishlal It seems that we can set a value via ctx.channel().attr(<key>).set(<value>);, can you take a look if it works?

@mqliang Good find, although we refactored the channelRead0 method to catch all exception so this is no longer needed. The comment is somewhat misleading now and I will update it. Note that an exception can occur before or while deserializing the message bytes also, so we may not have requestId in any case even if we set an attribute. It is probably better to handle all exceptions in channelRead0 method itself in any case.

mayankshriv · 2021-02-21T18:32:31Z

pinot-core/src/main/java/org/apache/pinot/core/transport/InstanceRequestHandler.java

+
+      _serverMetrics.addMeteredGlobalValue(ServerMeter.QUERY_EXECUTION_EXCEPTIONS, 1);
+    } catch (Throwable t) {
+      // Ignore since we are already handling a higher level exceptions.


But then we won't be sending a response back (ie line 140 may not execute)?

I think you are referring to the fact that byte[] serializedDataTable = dataTable.toBytes(); can throw an exception and that would prevent sendResponse from being called in the next line. The problem here is that we need to send byte data in sendResponse. One option may be to hardcode some serialized bytes and send that if an exception occurs?

At the very least, raise a metric.

mayankshriv · 2021-02-21T18:36:13Z

pinot-core/src/main/java/org/apache/pinot/core/transport/InstanceRequestHandler.java

+  /**
+   * Send an exception back to broker as response to the query request.
+   */
+  private void sendResponse(ChannelHandlerContext ctx, long requestId, long queryArrivalTimeMs, DataTable dataTable,


Not sure if I understand why we need to separate signatures for this? Can the caller not prepare the DaaTable as necessary for the response, (eg set the exception in the data table if there's one)? And this method is just responsible for sending back the response?

The onSuccess method in the callback gets byte data so to send that byte data back to server we need a sendResponse function that takes byte data as input. Then there are cases where we detect error conditions and need to create our own byte representation of the error condition. This is where the first sendResponse method is called. The first sendResponse method converts DataTable into byte data and calls the second more generic sendResponse method. I can rename the first sendResponse method to sendErrorResponse if that clarifies things better.

I would probably refactor this as two separate utility methods:

Building the response to send. This can have two flavors, one for success, and one for failure

Another one just for sending the response. And if this turns out to be just function call, then it might even be inline.

More of suggestion at this point, not a blocker for the PR.

I renamed the first function to sendErrorResponse for clarity. Although further refactoring can organize the code better, but the refactoring seems a bit outside the scope of this issue. For example, currently, the success (and some failure cases) inside QuerySchedular _queryScheduler.submit(queryRequest) and we get the response bytes back through a callback.

mayankshriv

Could you please also add a test (that should fail without your fix)?

…rror-response-latency

amrishlal · 2021-02-23T02:54:27Z

Could you please also add a test (that should fail without your fix)?

Added testServerErrorWithBrokerTimeout test case to OfflineClusterIntegrationTest.java

pinot-core/src/main/java/org/apache/pinot/core/query/scheduler/QueryScheduler.java

mayankshriv · 2021-02-23T17:12:56Z

pinot-core/src/main/java/org/apache/pinot/core/transport/InstanceRequestHandler.java

+  /**
+   * Send an exception back to broker as response to the query request.
+   */
+  private void sendResponse(ChannelHandlerContext ctx, long requestId, long queryArrivalTimeMs, DataTable dataTable,


I would probably refactor this as two separate utility methods:

Building the response to send. This can have two flavors, one for success, and one for failure

Another one just for sending the response. And if this turns out to be just function call, then it might even be inline.

More of suggestion at this point, not a blocker for the PR.

mayankshriv · 2021-02-23T17:14:13Z

pinot-core/src/main/java/org/apache/pinot/core/transport/InstanceRequestHandler.java

+
+      _serverMetrics.addMeteredGlobalValue(ServerMeter.QUERY_EXECUTION_EXCEPTIONS, 1);
+    } catch (Throwable t) {
+      // Ignore since we are already handling a higher level exceptions.


At the very least, raise a metric.

mayankshriv · 2021-02-23T17:16:45Z

...on-tests/src/test/java/org/apache/pinot/integration/tests/OfflineClusterIntegrationTest.java

@@ -438,6 +439,29 @@ public void testBloomFilterTriggering()
    }, 600_000L, "Failed to generate bloom filter");
  }

+  /** Check if server returns error response quickly without timing out Broker. */
+  @Test
+  public void testServerErrorWithBrokerTimeout()


Can we also add a test to ensure that if there are multiple requests, the right one gets the exception on the broker side. We discussed earlier in the PR where it was difficult to identify which request threw the exception?

https://github.com/apache/incubator-pinot/pull/6596/files#r579845996

QueryRoutingTest (unit) does some of this, but mocks the server away. I am not seeing a straightforward way of adding an integration test that would involve both server and broker since the data structures that map broker request to server response are deep down. Although, now that all exceptions are being handled within channelRead0 function, this should not be an issue.

siddharthteotia · 2021-02-24T19:06:24Z

pinot-core/src/main/java/org/apache/pinot/core/transport/InstanceRequestHandler.java

    try {
+      // put all inside try block to catch all exceptions.
+      final int requestSize = msg.readableBytes();
+


This is the style I also followed of using final for local variables for my first few PRs. Glad to know someone else did it too. However, Pinot coding style doesn't follow this. So you may want to remove it for now.

But something we can discuss independent of this PR. I think it makes sense to use final for constant local variables as well and not just instance variables.

siddharthteotia · 2021-02-24T19:06:28Z

pinot-core/src/main/java/org/apache/pinot/core/transport/InstanceRequestHandler.java

+      _serverMetrics.addMeteredGlobalValue(ServerMeter.NETTY_CONNECTION_BYTES_RECEIVED, requestSize);
+
+      // parse instance request into a query result.
+      msg.readBytes(requestBytes);


Typo? Should be parse instance request into a query request

siddharthteotia · 2021-02-24T19:06:32Z

pinot-core/src/main/java/org/apache/pinot/core/transport/InstanceRequestHandler.java

-      _serverMetrics.addMeteredGlobalValue(ServerMeter.REQUEST_DESERIALIZATION_EXCEPTIONS, 1);
-      return;
+      String hexString = requestBytes != null ? BytesUtils.toHexString(requestBytes) : "";
+      long reqestId = instanceRequest != null ? instanceRequest.getRequestId() : 0;


Isnt't this null check late? I think we should check this after lines 90 and 91 respectively to avoid NPE

We won't get a NPE for normal case because instanceRequest gets instantiated in line 82. If by chance an exception is thrown before that, we need a null check in the exception handler to avoid NPE. Same with requestBytes. Declaration is separate from instantiation and that is forcing Null check in exception handler.

My bad, I didn't notice the instantiation. Makes sense now

siddharthteotia · 2021-02-24T19:06:35Z

pinot-core/src/main/java/org/apache/pinot/core/transport/InstanceRequestHandler.java

+      String hexString = requestBytes != null ? BytesUtils.toHexString(requestBytes) : "";
+      long reqestId = instanceRequest != null ? instanceRequest.getRequestId() : 0;
+      LOGGER.error("Exception while deserializing the instance request: {}", hexString, e);
+      sendErrorResponse(ctx, reqestId, queryArrivalTimeMs, new DataTableImplV2(), e);


Looks like we are no longer emitting the metric REQUEST_DESERIALIZATION_EXCEPTIONS if we catch exception during deserialization ?

I see that sendErrorResponse doing the following
_serverMetrics.addMeteredGlobalValue(ServerMeter.QUERY_EXECUTION_EXCEPTIONS, 1);

I think we should continue to emit the request deserialization exception metric because I believe the above metric will be emitted even if the exception didn't happen during request deserialization

Added it back.

siddharthteotia · 2021-02-24T19:06:39Z

pinot-core/src/main/java/org/apache/pinot/core/transport/InstanceRequestHandler.java

        }
      }

      @Override
      public void onFailure(Throwable t) {
-        LOGGER.error("Caught exception while processing instance request", t);
-        _serverMetrics.addMeteredGlobalValue(ServerMeter.UNCAUGHT_EXCEPTIONS, 1);
+        // Send exception response.


Can you please also update the PR description to call out the metrics that will no longer be emitted after this PR. Looks like UNCAUGHT_EXCEPTIONS and REQUEST_FETCH_EXCEPTION are the ones

siddharthteotia · 2021-02-24T19:06:45Z

pinot-core/src/main/java/org/apache/pinot/core/transport/InstanceRequestHandler.java

+      Map<String, String> dataTableMetadata = dataTable.getMetadata();
+      dataTableMetadata.put(DataTable.REQUEST_ID_METADATA_KEY, Long.toString(requestId));
+
+      dataTable.addException(QueryException.getException(QueryException.QUERY_EXECUTION_ERROR, e));


(nit) remove empty line if not intentional

siddharthteotia · 2021-02-24T23:59:25Z

pinot-core/src/main/java/org/apache/pinot/core/transport/InstanceRequestHandler.java

-      return;
-    }
+      if (e instanceof TException) {
+        // deserialization exception


So this if check of the exception type guarantees that exception is coming from line 92 and not before or after that?

Yes, _deserializer.deserialize(instanceRequest, requestBytes) is the only method that throws TException. Also, any exceptions thrown within:

Futures.addCallback(_queryScheduler.submit(queryRequest), createCallback(ctx, queryArrivalTimeMs, instanceRequest, queryRequest), MoreExecutors.directExecutor());

will get handled by the callback's onFailure method.

siddharthteotia

LGTM. Thanks for fixing this.

amrishlal added 3 commits February 19, 2021 12:50

Always return a response from query execution.

2c4b57e

Cleanup.

f393185

Handle all exceptions in chanelRead0 method.

0e7a7d5

amrishlal changed the title ~~[WIP] Always return a response from query execution.~~ Always return a response from query execution. Feb 21, 2021

mayankshriv reviewed Feb 21, 2021

View reviewed changes

amrishlal added 6 commits February 21, 2021 22:59

codereview changes.

d35bbcd

codereview changes.

18a95d5

Rebuild.

6026803

Add test case.

24f8811

Add test case.

4e825cb

Merge branch 'master' of github.com:apache/incubator-pinot into fix-e…

2e684f4

…rror-response-latency

mayankshriv reviewed Feb 23, 2021

View reviewed changes

amrishlal added 2 commits February 23, 2021 16:10

code review changes.

775c25f

codereview changes.

b9378d5

siddharthteotia reviewed Feb 24, 2021

View reviewed changes

code review changes.

cad4e1f

siddharthteotia reviewed Feb 24, 2021

View reviewed changes

siddharthteotia approved these changes Feb 25, 2021

View reviewed changes

amrishlal added 2 commits February 24, 2021 16:22

Cleanup comments.

8c75e78

Rebuild.

86aee9a

siddharthteotia merged commit 98d569d into apache:master Feb 25, 2021

Always return a response from query execution. #6596

Always return a response from query execution. #6596

Conversation

amrishlal commented Feb 19, 2021 • edited

Description

Upgrade Notes

Release Notes

Documentation

codecov-io commented Feb 21, 2021 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mayankshriv left a comment

Choose a reason for hiding this comment

amrishlal commented Feb 23, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amrishlal Feb 24, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

siddharthteotia left a comment

Choose a reason for hiding this comment

amrishlal commented Feb 19, 2021 •

edited

codecov-io commented Feb 21, 2021 •

edited

amrishlal Feb 24, 2021 •

edited