New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not retry requests when response is too long #92

Merged
merged 1 commit into from Nov 8, 2018

Conversation

Projects
None yet
3 participants
@arhimondr
Contributor

arhimondr commented Nov 8, 2018

No description provided.

@arhimondr arhimondr force-pushed the arhimondr:do-not-retry-too-long-response branch from d7dcae2 to 2a02dcf Nov 8, 2018

@electrum

A few nits, otherwise looks good to me. @dain should also take a look

import io.airlift.drift.protocol.TTransportException;
public class TooLongMessageException

This comment has been minimized.

@electrum

electrum Nov 8, 2018

Member

This name sounds strange. How about MessageTooLargeException

@@ -211,6 +213,9 @@ private void onError(ChannelHandlerContext context, Throwable throwable, Optiona
if (throwable instanceof TException) {
thriftException = (TException) throwable;
}
else if (throwable instanceof TooLongFrameException) {
thriftException = new TooLongMessageException(throwable.getMessage(), throwable);

This comment has been minimized.

@electrum

electrum Nov 8, 2018

Member

No need to pass the message. That’s the default throwable behavior when you just pass the cause.

This comment has been minimized.

@arhimondr

arhimondr Nov 8, 2018

Contributor

That’s the default throwable behavior when you just pass the cause.

It includes the underlying exception type. So the message will be literally io.netty.handler.codec.TooLongFrameException: Adjusted frame length exceeds 16777216: 25444826 - discarded. Not sure if the io.netty.handler.codec.TooLongFrameException: is really usefull.

Ideally it would be nice to have our own message. The message from netty is confusing. But the TooLongFrameException doesn't include the numbers =\

@@ -108,6 +110,10 @@ public ExceptionClassification classifyException(Throwable throwable, boolean id
return new ExceptionClassification(Optional.of(TRUE), NORMAL);
}
if (throwable instanceof TooLongMessageException) {
return new ExceptionClassification(Optional.of(FALSE), NORMAL);

This comment has been minimized.

@electrum

electrum Nov 8, 2018

Member

Use FALSE to be consistent with the other code (doesn’t matter except for consistency)

@arhimondr arhimondr force-pushed the arhimondr:do-not-retry-too-long-response branch from 2a02dcf to 961e54d Nov 8, 2018

@dain

dain approved these changes Nov 8, 2018

@arhimondr arhimondr force-pushed the arhimondr:do-not-retry-too-long-response branch from 961e54d to bc61d7d Nov 8, 2018

@electrum

This comment has been minimized.

Member

electrum commented Nov 8, 2018

I just realized we should name the exception FrameTooLargeException since that matches the max-frame-size config

@arhimondr arhimondr force-pushed the arhimondr:do-not-retry-too-long-response branch from bc61d7d to ed4e47f Nov 8, 2018

@dain dain merged commit ebd5573 into airlift:master Nov 8, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@arhimondr arhimondr deleted the arhimondr:do-not-retry-too-long-response branch Nov 8, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment