Fix exception cause logging in QueryResultPusher by kgyrtkirk · Pull Request #14975 · apache/druid

kgyrtkirk · 2023-09-13T17:17:28Z

QueryResultPusher started clipping exception causes since #14004 was merged

cfd07a9#diff-8af2383f47d5423babf455ad7ec503ea8196e88b8b664e1d1e179163a9931f9bR211

pranavbhole · 2023-09-13T17:44:13Z

org.apache.druid.error.DruidException: null
	at org.apache.druid.error.DruidException$DruidExceptionBuilder.build(DruidException.java:454) ~[druid-processing-2023.08.0-iap.jar:2023.08.0-iap]
	at org.apache.druid.error.DruidException$DruidExceptionBuilder.build(DruidException.java:444) ~[druid-processing-2023.08.0-iap.jar:2023.08.0-iap]
	at org.apache.druid.error.QueryExceptionCompat.makeException(QueryExceptionCompat.java:50) ~[druid-processing-2023.08.0-iap.jar:2023.08.0-iap]
	at org.apache.druid.error.DruidException.fromFailure(DruidException.java:154) ~[druid-processing-2023.08.0-iap.jar:2023.08.0-iap]
	at org.apache.druid.server.QueryResultPusher.handleQueryException(QueryResultPusher.java:211) ~[druid-server-2023.08.0-iap.jar:2023.08.0-iap]
	at org.apache.druid.server.QueryResultPusher.push(QueryResultPusher.java:177) ~[druid-server-2023.08.0-iap.jar:2023.08.0-iap]
	at org.apache.druid.server.QueryResource.doPost(QueryResource.java:218) ~[druid-server-2023.08.0-iap.jar:2023.08.0-iap]
	at jdk.internal.reflect.GeneratedMethodAccessor207.invoke(Unknown Source) ~[?:?]
	at jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) ~[?:?]
	at java.lang.reflect.Method.invoke(Method.java:566) ~[?:?]
	at com.sun.jersey.spi.container.JavaMethodInvokerFactory$1.invoke(JavaMethodInvokerFactory.java:60) ~[jersey-server-1.19.4.jar:1.19.4]
	at com.sun.jersey.server.impl.model.method.dispatch.AbstractResourceMethodDispatchProvider$ResponseOutInvoker._dispatch(AbstractResourceMethodDispatchProvider.java:205) ~[jersey-server-1.19.4.jar:1.19.4]
	at com.sun.jersey.server.impl.model.method.dispatch.ResourceJavaMethodDispatcher.dispatch(ResourceJavaMethodDispatcher.java:75) ~[jersey-server-1.19.4.jar:1.19.4]
	at com.sun.jersey.server.impl.uri.rules.HttpMethodRule.accept(HttpMethodRule.java:302) ~[jersey-server-1.19.4.jar:1.19.4]
	at com.sun.jersey.server.impl.uri.rules.ResourceClassRule.accept(ResourceClassRule.java:108) ~[jersey-server-1.19.4.jar:1.19.4]
	at com.sun.jersey.server.impl.uri.rules.RightHandPathRule.accept(RightHandPathRule.java:147) ~[jersey-server-1.19.4.jar:1.19.4]
	at

pranavbhole · 2023-09-13T18:09:50Z

+    if (!(e instanceof QueryException)) {
+      e = new QueryInterruptedException(e);
+    }
+    return DruidException.fromFailure(new QueryExceptionCompat((QueryException) e));


(QueryException) is it safe cast?

QueryInterruptedException extends QueryException. So should be good

yes - actually; I've changed the way this is fixed

I now think that it was not intentional that QueryExceptionCompat have not passed the exception ; only it's message

cc: @cheddar

somu-imply · 2023-09-13T18:58:25Z

+      @Override
+      public void writeException(Exception e, OutputStream out) throws IOException
+      {
+        throw new RuntimeException("Unimplemented!");


maybe just do a
out.write(jsonMapper.writeValueABytes(e))
here

the test was not covering this method - as it was already pretty awkward to write a mocked test for this class...

I've extended the test to cover this method as well... I had to add 4 when-s/etc :)

This reverts commit c863779.

imply-cheddar · 2023-09-14T10:55:16Z

The QueryCompat stuff is a compatibility layer with the way that the old QueryException worked and an attempt to map it onto DruidException.

QueryExceptions are/were exceptions that are serialized across the wire, deserialized in the brokers and then converted back into exceptions. IIRC, they don't include the stack traces because the historical-side stacktrace doesn't make sense on the broker.

The correct approach to fix odd exception reporting here is to go to the source of the exception and turn it into a meaningful DruidException which will cause the higher stacks to use the DruidException handling code instead.

imply-cheddar · 2023-09-14T10:57:31Z

Also, the ResultPusher classes are tested "through" QueryResourceTest and SqlQueryResourceTest respectively. QueryResultPusher is an abstract class that's just trying to make the logic of the Resources similar, but the thing that actually matters is that the Resources are operating according to the API contract. Anyway, switching around to trying to test through one of those tests will perhaps make it easier.

imply-cheddar · 2023-09-14T12:56:39Z

That said, I just double checked and adding the cause shouldn't actually change anything with wire serialization. So that should be fine and having it log-able is good. Let me provide some comments on the tests.

imply-cheddar

The actual change is good. Please try to remove the mocks from the test.

imply-cheddar · 2023-09-14T12:58:38Z

+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;


Generally speaking, we try to use mocks sparingly. And, Mockito is the less-preferred of libraries versus EasyMock (we had a test once that used Mockito to mock a ByteBuffer which 4x'd the runtime of all tests because it ended up rewriting the byte code of ByteBuffer which slowed down all other tests in that JVM).

It was an interesting excercise to rewrite this test to use EasyMock instead; its definetly different.

imply-cheddar · 2023-09-14T13:03:01Z

+    ObjectMapper jsonMapper = new ObjectMapper();
+    ResponseContextConfig responseContextConfig = ResponseContextConfig.newConfig(true);
+    DruidNode selfNode = mock(DruidNode.class);
+    QueryResource.QueryMetricCounter counter = mock(QueryResource.QueryMetricCounter.class);


Just make a local public static class NoopQueryMetricCounter and new that up. Your IDE should be able to generate it all with just a few keystrokes.

all implementations of QueryMetricCounter are private - so I had to retain the mock here
there is SqlResource.QUERY_METRIC_COUNTER ; but that's in the sql module; so I can't use that here

kgyrtkirk · 2023-09-15T08:33:33Z

I've also done a manual before/after check - to see if this fully fixes that the Exception is not clipped out from the historical logs - and that's fixed with this patch (no more ducks eating exceptions) :D

rohangarg

LGTM

…ause-fix

rohangarg · 2023-09-20T10:15:40Z

The changes are good, the CI failures are due to some flakiness in the ITs. We re-triggered them multiple times but were unsuccessful in getting them passed. Merging on the basis that the build was green on a commit earlier, the newer commits are either changes to UTs which wouldn't affect the IT flow or master merges

kgyrtkirk · 2023-09-20T10:18:31Z

thank you @rohangarg for the review and merging these changes!

kgyrtkirk added 6 commits September 13, 2023 14:10

stuff

414d5c5

stu

8eeda44

fix0

a44940a

updates

3c8fa61

remove unrelated

738ad56

fix checkstyle

6d19e14

kgyrtkirk marked this pull request as ready for review September 13, 2023 17:50

pranavbhole reviewed Sep 13, 2023

View reviewed changes

somu-imply reviewed Sep 13, 2023

View reviewed changes

kgyrtkirk added 6 commits September 14, 2023 07:40

bad idea

c863779

Revert "bad idea"

97ab0c6

This reverts commit c863779.

update test to check #writeException

5bbd852

cleanup

065225a

warnings

d9c0fac

alternate fix

5571b08

more test

915e370

imply-cheddar approved these changes Sep 14, 2023

View reviewed changes

kgyrtkirk added 6 commits September 14, 2023 13:14

fix style

6069b10

different approach

b8ede46

update tests

2831662

remove static import

f5d94df

update test

c86fe5c

cleanup

1f74c8d

add NoopQueryMetricCounter

ee1e1fa

rohangarg approved these changes Sep 15, 2023

View reviewed changes

kgyrtkirk added 3 commits September 18, 2023 14:08

Merge remote-tracking branch 'apache/master' into queryResultPusher-c…

237065d

…ause-fix

empty-commit+merge to retrigger ci

9791b9d

Merge remote-tracking branch 'apache/master' into queryResultPusher-c…

1433dc1

…ause-fix

rohangarg merged commit 79f882f into apache:master Sep 20, 2023

LakshSingla added this to the 28.0 milestone Oct 12, 2023

Conversation

kgyrtkirk commented Sep 13, 2023

Uh oh!

pranavbhole commented Sep 13, 2023

Uh oh!

pranavbhole Sep 13, 2023

Choose a reason for hiding this comment

Uh oh!

somu-imply Sep 13, 2023

Choose a reason for hiding this comment

Uh oh!

kgyrtkirk Sep 14, 2023

Choose a reason for hiding this comment

Uh oh!

somu-imply Sep 13, 2023 • 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!

kgyrtkirk Sep 14, 2023

Choose a reason for hiding this comment

Uh oh!

imply-cheddar commented Sep 14, 2023

Uh oh!

imply-cheddar commented Sep 14, 2023

Uh oh!

imply-cheddar commented Sep 14, 2023

Uh oh!

imply-cheddar left a comment

Choose a reason for hiding this comment

Uh oh!

imply-cheddar Sep 14, 2023

Choose a reason for hiding this comment

Uh oh!

kgyrtkirk Sep 14, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

imply-cheddar Sep 14, 2023

Choose a reason for hiding this comment

Uh oh!

kgyrtkirk Sep 14, 2023 • 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!

Uh oh!

Uh oh!

Uh oh!

kgyrtkirk commented Sep 15, 2023

Uh oh!

rohangarg left a comment

Choose a reason for hiding this comment

Uh oh!

rohangarg commented Sep 20, 2023

Uh oh!

kgyrtkirk commented Sep 20, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

somu-imply Sep 13, 2023 •

edited

Loading

kgyrtkirk Sep 14, 2023 •

edited

Loading