hadoop-hdfs-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Haohui Mai (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (HDFS-8515) Abstract a DTP/2 HTTP/2 server
Date Tue, 23 Jun 2015 23:39:42 GMT

    [ https://issues.apache.org/jira/browse/HDFS-8515?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14598578#comment-14598578
] 

Haohui Mai commented on HDFS-8515:
----------------------------------

It might make sense to move all the code into the {o.a.h.web.http2} package.
{code}
+
+  // whether to log http2 frame for debugging
+  public static final String  DFS_HTTP2_VERBOSE_KEY = "dfs.http2.verbose";
+  public static final boolean DFS_HTTP2_VERBOSE_DEFAULT = false;

+    if (verbose) {
+      frameReader =
+          new Http2InboundFrameLogger(new DefaultHttp2FrameReader(),
+              FRAME_LOGGER);
+      frameWriter =
+          new Http2OutboundFrameLogger(new DefaultHttp2FrameWriter(),
+              FRAME_LOGGER);

{code}

Instead of adding a new configuration, a better approach might be adding a logger into {{ServerHttp2ConectionHandler}}
and check whether the debug log is enabled.

{code}
+  private static final ChannelMetadata METADATA = new ChannelMetadata(false);
+
+  private final ChannelHandlerContext http2ConnHandlerCtx;
+
+  private final Http2Stream connStream;
+
+  private final Http2Stream stream;
+
{code}

There should be no empty lines between these members.

{code}
+  private final Http2LocalFlowController localFlowController;
+
+  private final Http2RemoteFlowController remoteFlowController;
+
{code}

It might make sense to separate the flow control logic into a separate patch.

{code}
+        encoder.writeHeaders(http2ConnHandlerCtx, stream.id(),
+          (Http2Headers) msg, 0, endOfStream, http2ConnHandlerCtx.newPromise());
{code}

encoder is not thread-safe. It seems to me the right approach is to run the write in the event
loop of the parent channel. The read path might have the same issue.

{code}
+public class LastChunkedInput implements ChunkedInput<Object> {
+public class LastMessage {
{code}

To me both {{LastChunkedInput}} and {{LastMessage}} look like more of an optimization right
now. A simpler approach is to send an empty HEADER with the end-of-stream bit on to tell the
remote peer that the stream has been closed.

{code}
+public abstract class AbstractTestHttp2Server {
{code}

It can be a utility class instead of asking all HTTP2 test cases to inherit it.

{code}
+public class TestHttp2ServerMultiThread extends AbstractTestHttp2Server {
{code}

I'm yet to be convinced that testing of mutli threading is required right now. Maybe having
some coverage of the basic funciationlities is a higher priority.


> Abstract a DTP/2 HTTP/2 server
> ------------------------------
>
>                 Key: HDFS-8515
>                 URL: https://issues.apache.org/jira/browse/HDFS-8515
>             Project: Hadoop HDFS
>          Issue Type: Sub-task
>            Reporter: Duo Zhang
>            Assignee: Duo Zhang
>         Attachments: HDFS-8515-v1.patch, HDFS-8515-v2.patch, HDFS-8515-v3.patch, HDFS-8515-v4.patch,
HDFS-8515.patch
>
>
> Discussed in HDFS-8471.
> https://issues.apache.org/jira/browse/HDFS-8471?focusedCommentId=14568196&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14568196



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Mime
View raw message