Unify http channels and exception handling #31379

Tim-Brooks · 2018-06-15T23:18:50Z

This is a general cleanup of channels and exception handling in http.
This commit introduces a CloseableChannel that is a superclass of
TcpChannel and HttpChannel. This allows us to unify the closing logic
between tcp and http transports. Additionally, the normal http channels
are extracted to the abstract server transport.

Finally, this commit (mostly) unifies the exception handling between nio
and netty4 http server transports.

This is a general cleanup of channels and exception handling in http. This commit introduces a CloseableChannel that is a superclass of TcpChannel and HttpChannel. This allows us to unify the closing logic between tcp and http transports. Additionally, the normal http channels are extracted to the abstract server transport. Finally, this commit (mostly) unifies the exception handling between nio and netty4 http server transports.

elasticmachine · 2018-06-15T23:18:52Z

Pinging @elastic/es-core-infra

Tim-Brooks · 2018-06-15T23:19:14Z

While working on this PR I had two issues that I thought were worth raising:

Right now in http server transport we log a close channel exception at warning level. In the tcp transport we log that at trace level. In the http server transport we log unidentified channel exceptions at debug level. In tcp transport we log that at a warn level. Do we want to have these logged at the same level?
In the http stats api we return:

server channels
total channels

I interpret that as server channels are the number of bound open channels that are accepting connections. And total channels are those server channels + the number of channels that have been accepted.

But it looks like currently in the netty http transport it is returning:

server channels = number of currently open accepted channels
total channels = number of channels that have been accepted since the process started (even if the channel is now closed).

Thoughts on what the correct behavior is?

@s1monw @jasontedor

s1monw · 2018-06-17T13:51:17Z

Right now in http server transport we log a close channel exception at warning level. In the tcp transport we log that at trace level. In the http server transport we log unidentified channel exceptions at debug level. In tcp transport we log that at a warn level. Do we want to have these logged at the same level?

I think we should:

log close channel exceptions at trace level everywhere
log unidentified channel exceptions at warn level.

But, I wonder if there were reasons for this we don't know maybe @kimchy knows?

In the http stats api we return:

to me the existing behavior makes sense?

s1monw

I left some quesitons, LGTM otherwise

s1monw · 2018-06-17T13:52:03Z

.../transport-netty4/src/main/java/org/elasticsearch/http/netty4/Netty4HttpServerTransport.java

        }
+        httpChannels.clear();


would be cleaner if we do it in a finally?

s1monw · 2018-06-17T13:54:35Z

server/src/main/java/org/elasticsearch/common/network/CloseableChannel.java

+     * to implement logic that depends on knowing when the channel is closed.
+     */
+    @Override
+    void close();


hmm should this declare IOException?

No. Since the close call is just queuing a channel for close (exception for the blocking mock transport) there is no IOException.

ok maybe leave a comment

s1monw · 2018-06-17T13:55:00Z

server/src/main/java/org/elasticsearch/common/network/CloseableChannel.java

+                if (channel.isOpen()) {
+                    PlainActionFuture<Void> closeFuture = PlainActionFuture.newFuture();
+                    channel.addCloseListener(closeFuture);
+                    channel.close();


if this close fails should we continue with the others?

s1monw · 2018-06-17T13:55:35Z

server/src/main/java/org/elasticsearch/common/network/CloseableChannel.java

+                // close exceptions happens elsewhere.
+            } catch (InterruptedException e) {
+                Thread.currentThread().interrupt();
+                throw new IllegalStateException("Future got interrupted", e);


wouldn't it be ok to just continue if we get interrupted?

So just this?

static void blockOnFutures(List<ActionFuture<Void>> futures) { for (ActionFuture<Void> future : futures) { try { future.get(); } catch (ExecutionException e) { // Ignore as we are only interested in waiting for the close process to complete. Logging // close exceptions happens elsewhere. } catch (InterruptedException e) { Thread.currentThread().interrupt(); } } }

yeah I wonder if that is hte right thing?

Tim-Brooks · 2018-06-18T04:25:39Z

I adjusted the PR with your logging level suggestions, what you expect the stats to show, and your specific comments.

s1monw

LGTM left one comment about the new utils method

s1monw · 2018-06-19T10:34:15Z

server/src/main/java/org/elasticsearch/common/network/CloseableChannel.java

@@ -83,19 +83,15 @@
     * @param blocking indicates if we should block on channel close
     */
    static <C extends CloseableChannel> void closeChannels(List<C> channels, boolean blocking) {
+        IOUtils.closeAndConvertToRuntimeExceptions(channels);


wouldnt' it be simpler to do:

try { IOUtils.close(channels); } catch (IOException e) { throw new UncheckedIOException(e); }

???

Sure. I’ll pull out the utils part and do that.

* master: [DOCS] Omit shard failures assertion for incompatible responses (#31430) [DOCS] Move licensing APIs to docs (#31445) Add Delete Snapshot High Level REST API Remove QueryCachingPolicy#ALWAYS_CACHE (#31451) [Docs] Extend Homebrew installation instructions (#28902) Choose JVM options ergonomically [Docs] Mention ip_range datatypes on ip type page (#31416) Multiplexing token filter (#31208) Fix use of time zone in date_histogram rewrite (#31407) Core: Remove index name resolver from base TransportAction (#31002) [DOCS] Fixes code snippet testing for machine learning (#31189) [DOCS] Removed and params from MLT. Closes #28128 (#31370) Security: fix joining cluster with production license (#31341) Unify http channels and exception handling (#31379) [DOCS] Moves the info API to docs (#31121) Preserve response headers on cluster update task (#31421) [DOCS] Add code snippet testing for more ML APIs (#31404) Do not preallocate bytes for channel buffer (#31400) Docs: Advice for reindexing many indices (#31279) Mute HttpExporterTests#testHttpExporterShutdown test Tracked by #31433 Docs: Add note about removing prepareExecute from the java client (#31401) Make release notes ignore the `>test-failure` label. (#31309)

Tim-Brooks added >non-issue review :Distributed Coordination/Network Http and internode communication implementations v7.0.0 labels Jun 15, 2018

Tim-Brooks requested review from s1monw, jaymode and jasontedor June 15, 2018 23:18

s1monw requested changes Jun 17, 2018

View reviewed changes

Changes from review

8c6e03b

Tim-Brooks requested a review from s1monw June 18, 2018 04:25

s1monw approved these changes Jun 19, 2018

View reviewed changes

Changes from review

71f288b

Tim-Brooks merged commit 529e704 into elastic:master Jun 19, 2018

Tim-Brooks deleted the cleanup_channels branch December 10, 2018 16:21

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unify http channels and exception handling #31379

Unify http channels and exception handling #31379

Tim-Brooks commented Jun 15, 2018

elasticmachine commented Jun 15, 2018

Tim-Brooks commented Jun 15, 2018

s1monw commented Jun 17, 2018

s1monw left a comment

s1monw Jun 17, 2018

s1monw Jun 17, 2018

Tim-Brooks Jun 17, 2018

s1monw Jun 19, 2018

s1monw Jun 17, 2018

s1monw Jun 17, 2018

Tim-Brooks Jun 17, 2018 •

edited

Loading

s1monw Jun 17, 2018

Tim-Brooks commented Jun 18, 2018

s1monw left a comment

s1monw Jun 19, 2018

Tim-Brooks Jun 19, 2018

Unify http channels and exception handling #31379

Unify http channels and exception handling #31379

Conversation

Tim-Brooks commented Jun 15, 2018

elasticmachine commented Jun 15, 2018

Tim-Brooks commented Jun 15, 2018

s1monw commented Jun 17, 2018

s1monw left a 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

Tim-Brooks Jun 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tim-Brooks commented Jun 18, 2018

s1monw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tim-Brooks Jun 17, 2018 •

edited

Loading