HDDS-5480. [Ozone-Streaming] Client and server should support stream setup. #2452

captainzmc · 2021-07-22T12:28:17Z

What changes were proposed in this pull request?

Before we add new BlockDataStreamOutput, we need to support stream setup. In this way, we can get the DataStreamOutput instance through xceiverClient.

We can DataStreamOutput instance as we used in our POC code.

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-5480

How was this patch tested?

CI test will be added after DataStreamOutput Client is finished

hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/ratis/RatisHelper.java

mukul1987 · 2021-07-22T12:34:29Z

hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/XceiverClientGrpc.java

    return responseProtoHashMap;
  }

+  // TODO: We need separate XceiverClientRatis and XceiverClientGrpc instances


Lets create a jira for this

mukul1987 · 2021-07-22T13:32:37Z

hadoop-hdds/common/src/main/resources/ozone-default.xml

  </property>
+  <property>
+    <name>dfs.container.ratis.datastream.ipc</name>
+    <value>9890</value>


These values are different from the values in the java conf file.

I will fix the problem and we should not use port 9890. This will conflict with the port in docker-compose

mukul1987 · 2021-07-22T13:34:43Z

hadoop-hdds/interface-client/src/main/proto/DatanodeClientProtocol.proto

  CloseContainer = 17;
  GetCommittedBlockLength = 18;
+
+  StreamInit = 19;


lets start this at 30.

I tried to change this to 30, but I got an NPE exception when start datanode. Change this to 19 in order and it start normal.

mukul1987 · 2021-07-22T13:35:16Z

...iner-service/src/main/java/org/apache/hadoop/ozone/container/common/impl/HddsDispatcher.java

            && dispatcherContext.getStage()
-            == DispatcherContext.WriteChunkStage.WRITE_DATA);
+            == DispatcherContext.WriteChunkStage.WRITE_DATA)
+            || (cmdType == Type.StreamInit);


are there any callers sending stream init at this time ?

Not yet now. But next step, when our client initializes the DataStreamOutput, we will set CmdType to type.streamInit. As we implement in our POC code.

mukul1987 · 2021-07-22T14:38:41Z

One more question, whats the use of stream init, and how is it different from the first data packet to be sent from client to the datanode ?

bshashikant · 2021-07-22T14:37:19Z

...java/org/apache/hadoop/ozone/container/common/transport/server/ratis/XceiverServerRatis.java

+    // set the datastream config
+    NettyConfigKeys.DataStream.setPort(properties, dataStreamPort);
+    RaftConfigKeys.DataStream.setType(properties,
+        SupportedDataStreamType.NETTY);


can we do these stream specfic settings in a different function?

bshashikant · 2021-07-22T14:40:25Z

...tainer-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java

+      }
+
+      path = chunkManager
+          .streamInit(kvContainer, blockID, dispatcherContext);


I feel, we should set the dispacher contect as null here. Dispacher context being null during write will signify its a stream opeartion.

Also, stream classes should be coupled with ChunkManager. Let's maintain a clear separation. May be we can use StreamHandler. Common functionalities can be moved to chunkUtils.

I would try to change the current implementation in this way.

bshashikant · 2021-07-22T14:41:19Z

hadoop-hdds/interface-client/src/main/proto/DatanodeClientProtocol.proto

 message  WriteChunkRequestProto  {
  required DatanodeBlockID blockID = 1;
-  required ChunkInfo chunkData = 2;
+  optional ChunkInfo chunkData = 2;


isn't this backword incompatible?

Changed this from required to optional is compatible. The previous interface WriteChunkRequestProto must have chunkData. Init stream will use WriteChunkRequestProto, but do not require chunkData. Just like we implemented in the POC code.

bshashikant · 2021-07-22T14:44:19Z

hadoop-hdds/common/src/main/resources/ozone-default.xml

+  <property>
+    <name>dfs.container.ratis.datastream.request.threads</name>
+    <value>20</value>
+    <tag>OZONE, CONTAINER, RATIS, DATASTREAM</tag>


Cab we move these settings to DatanodeRatisServerConfig and clinet config keys separately?

Had moved datastream.request.threads and datastream.write.threads to DatanodeRatisServerConfig. The rest of the configurations are similar to those related to Container, and I keep them in this place for better management.

bshashikant · 2021-07-22T14:45:52Z

hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/XceiverClientGrpc.java

+  @Override
+  public DataStreamApi getDataStreamApi() {
+    return null;
+  }


Its better to throw an exception here.

bshashikant · 2021-07-22T14:52:47Z

Thanks @captainzmc for working on this. I think, we need define a config option to enable/disable the feature both on client and server.

bshashikant · 2021-07-22T15:32:44Z

One more question, whats the use of stream init, and how is it different from the first data packet to be sent from client to the datanode ?

The strream init will be used to instantiate the streaming client instance and the will try to write the 1st packet.

kaijchen · 2021-07-23T03:02:09Z

One more question, whats the use of stream init, and how is it different from the first data packet to be sent from client to the datanode ?

Stream init do 2 things: build the stream pipeline, and get a file path from the container for subsequent writes.

Ratis: Build the stream pipeline according to the routing table. If no routing table is given, it will fallback to

Client -----> Primary -----> Secondary 1
                 |
                 +---------> Secondary 2

HDDS Container: prepare a file for write, but does not write any data. In our POC code, we let the ContainerStateMachine calculate the path of that file as if there is a WriteChunk request.

The first data packet, of course, write data into that file.

captainzmc · 2021-07-23T03:38:05Z

Thanks @captainzmc for working on this. I think, we need define a config option to enable/disable the feature both on client and server.

Thanks @bshashikant for the review and suggestions.

On the server side, I think it's easy to distinguish. We can added a config and check in XceiverServerRatis , to confirm whether eanble stream in RaftServer.
About the client. Can we write a key based on key size and decide whether to use stream? For example, we can add a configure and check this in putKeyHandler.java. If the key is smaller than the chunk size（4MB）, follow the original logic. If greater than, then use streaming.
And if the user uses the API directly, they can call our new interface directly. We won't make any changes to the existing Key/Block OutPutStream. We will add some new interfaces as described in the previous design. We also need to add new interfaces to RpcClient and OzoneBucket to use these interfaces.

captainzmc · 2021-07-23T07:44:14Z

I will solve the remaining review and CI problems.

szetszwo · 2021-07-26T16:38:18Z

@captainzmc , Just have went through the entire change once. The change looks great! Will review this again tomorrow.

In the mean time, please take a look the build failures. Thanks a lot.

captainzmc · 2021-07-27T02:13:55Z

@captainzmc , Just have went through the entire change once. The change looks great! Will review this again tomorrow.

In the mean time, please take a look the build failures. Thanks a lot.

Yes, now I am solving the problem of CI failure, and I will update this PR again after solving it.

szetszwo

Thanks for the update. Some comments inlined.

szetszwo · 2021-07-27T05:54:23Z

hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/XceiverClientGrpc.java

getDataStreamApi is yet not used. Let's add it later when it is being called.

BTW, we may only add the method to XceiverClientRatis and cast the object for calling it as below.

XceiverClientSpi client = ... DataStreamApi streamApi = ((XceiverClientRatis)client).getDataStreamApi();

Yes, we can add this when we implement the client

szetszwo · 2021-07-27T05:55:14Z

hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/protocol/DatanodeDetails.java

Let's call it RATIS_DATASTREAM.

szetszwo · 2021-07-27T05:59:09Z

hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/ratis/RatisHelper.java

Let's use the first node:

toRaftPeer(pipeline.getFirstNode()),

szetszwo · 2021-07-27T06:03:44Z

hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneConfigKeys.java

Let's remove "ipc", i.e. dfs.container.ratis.datastream.random.port". It is not really an ipc.

szetszwo · 2021-07-27T06:04:05Z

hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneConfigKeys.java

Let's rename this to "dfs.container.ratis.datastream.port".

szetszwo · 2021-07-27T06:05:36Z

...java/org/apache/hadoop/ozone/container/common/transport/server/ratis/XceiverServerRatis.java

Let's revert this whitespace change.

szetszwo

The change looks good. Just a minor comment inlined.

build-branch / acceptance (misc) (pull_request) Failing after 86m — acceptance (misc)

Please also take a look the build failure. Thanks.

szetszwo · 2021-07-27T13:55:17Z

hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/ratis/RatisHelper.java

+    // TODO: For now, always set first node as primary. In the future we should
+    //  use the local or closest node in the pipline.
+    final DatanodeDetails leader = pipeline.getLeaderNode();


The TODO can be removed since the first datanode is supposed to be the closest datanode. SCM is supposed to sort the datanode in the pipeline according the distance to the client. Not sure if SCM is currently sorting the datanodes. We should check the code.

BTW, the leader local variable is no longer needed. We should revert the change.

captainzmc · 2021-07-28T05:01:21Z

Hi @szetszwo @bshashikant @mukul1987 CI had been fixed. Could you help take another look？

szetszwo

+1 the change looks good.

…setup. (#2452)

…setup. (apache#2452)

…setup. (#2452)

…setup. (apache#2452)

…setup. (#2452) (cherry picked from commit 5230630) (cherry picked from commit 0a4bbf5c7b1d7190df818c3828110fcba4261704)

…setup. (#2452) (cherry picked from commit 5230630) (cherry picked from commit 0a4bbf5c7b1d7190df818c3828110fcba4261704) (cherry picked from commit 0568725)

…setup. (#2452) (cherry picked from commit c69955e)

…setup. (apache#2452) (cherry picked from commit c69955e)

captainzmc requested a review from szetszwo July 22, 2021 12:29

captainzmc closed this Jul 22, 2021

captainzmc reopened this Jul 22, 2021

mukul1987 requested changes Jul 22, 2021

View reviewed changes

bshashikant reviewed Jul 22, 2021

View reviewed changes

captainzmc force-pushed the init-stream branch from e8a9ad1 to e5874f4 Compare July 23, 2021 06:05

captainzmc force-pushed the init-stream branch 2 times, most recently from c2e0c79 to b9b1fec Compare July 23, 2021 08:09

szetszwo reviewed Jul 27, 2021

View reviewed changes

captainzmc force-pushed the init-stream branch 2 times, most recently from 3cf6b7e to 6616cd5 Compare July 27, 2021 08:19

szetszwo reviewed Jul 27, 2021

View reviewed changes

captainzmc force-pushed the init-stream branch from f112e16 to 7ef6e60 Compare July 27, 2021 14:38

captainzmc closed this Jul 27, 2021

captainzmc reopened this Jul 27, 2021

szetszwo approved these changes Jul 28, 2021

View reviewed changes

szetszwo merged this pull request into apache:HDDS-4454 Jul 28, 2021

szetszwo pushed a commit that referenced this pull request Jul 28, 2021

HDDS-5480. [Ozone-Streaming] Client and server should support stream …

faab6e7

…setup. (#2452)

szetszwo pushed a commit that referenced this pull request Aug 26, 2021

HDDS-5480. [Ozone-Streaming] Client and server should support stream …

bbfc4c0

…setup. (#2452)

szetszwo pushed a commit that referenced this pull request Sep 8, 2021

HDDS-5480. [Ozone-Streaming] Client and server should support stream …

2ef30bb

…setup. (#2452)

szetszwo pushed a commit that referenced this pull request Oct 1, 2021

HDDS-5480. [Ozone-Streaming] Client and server should support stream …

8df196a

…setup. (#2452)

szetszwo pushed a commit that referenced this pull request Oct 19, 2021

HDDS-5480. [Ozone-Streaming] Client and server should support stream …

501dc12

…setup. (#2452)

szetszwo pushed a commit that referenced this pull request Oct 28, 2021

HDDS-5480. [Ozone-Streaming] Client and server should support stream …

b9368a5

…setup. (#2452)

szetszwo pushed a commit that referenced this pull request Nov 15, 2021

HDDS-5480. [Ozone-Streaming] Client and server should support stream …

51d1013

…setup. (#2452)

szetszwo pushed a commit that referenced this pull request Nov 30, 2021

HDDS-5480. [Ozone-Streaming] Client and server should support stream …

91a4d82

…setup. (#2452)

captainzmc added a commit that referenced this pull request Dec 20, 2021

HDDS-5480. [Ozone-Streaming] Client and server should support stream …

d84c201

…setup. (#2452)

szetszwo pushed a commit that referenced this pull request Dec 30, 2021

HDDS-5480. [Ozone-Streaming] Client and server should support stream …

27b112f

…setup. (#2452)

szetszwo pushed a commit that referenced this pull request Jan 20, 2022

HDDS-5480. [Ozone-Streaming] Client and server should support stream …

4de1725

…setup. (#2452)

captainzmc added a commit that referenced this pull request Feb 9, 2022

HDDS-5480. [Ozone-Streaming] Client and server should support stream …

923c843

…setup. (#2452)

szetszwo pushed a commit that referenced this pull request Feb 16, 2022

HDDS-5480. [Ozone-Streaming] Client and server should support stream …

1abee11

…setup. (#2452)

szetszwo pushed a commit that referenced this pull request Mar 15, 2022

HDDS-5480. [Ozone-Streaming] Client and server should support stream …

decaa57

…setup. (#2452)

szetszwo pushed a commit that referenced this pull request Mar 24, 2022

HDDS-5480. [Ozone-Streaming] Client and server should support stream …

c554508

…setup. (#2452)

szetszwo pushed a commit to szetszwo/ozone that referenced this pull request May 6, 2022

HDDS-5480. [Ozone-Streaming] Client and server should support stream …

14856e8

…setup. (apache#2452)

szetszwo pushed a commit that referenced this pull request May 13, 2022

HDDS-5480. [Ozone-Streaming] Client and server should support stream …

354cc3e

…setup. (#2452)

szetszwo pushed a commit that referenced this pull request May 24, 2022

HDDS-5480. [Ozone-Streaming] Client and server should support stream …

968d1f1

…setup. (#2452)

szetszwo pushed a commit that referenced this pull request Jun 9, 2022

HDDS-5480. [Ozone-Streaming] Client and server should support stream …

4c93359

…setup. (#2452)

captainzmc added a commit to captainzmc/hadoop-ozone that referenced this pull request Jul 4, 2022

HDDS-5480. [Ozone-Streaming] Client and server should support stream …

5230630

…setup. (apache#2452)

szetszwo pushed a commit that referenced this pull request Oct 25, 2022

HDDS-5480. [Ozone-Streaming] Client and server should support stream …

0568725

…setup. (#2452) (cherry picked from commit 5230630) (cherry picked from commit 0a4bbf5c7b1d7190df818c3828110fcba4261704)

szetszwo pushed a commit that referenced this pull request Dec 1, 2022

HDDS-5480. [Ozone-Streaming] Client and server should support stream …

0c4105a

…setup. (#2452) (cherry picked from commit c69955e)

szetszwo pushed a commit that referenced this pull request Dec 16, 2022

HDDS-5480. [Ozone-Streaming] Client and server should support stream …

da95602

…setup. (#2452) (cherry picked from commit c69955e)

nishitpatira pushed a commit to nishitpatira/ozone that referenced this pull request Dec 16, 2022

HDDS-5480. [Ozone-Streaming] Client and server should support stream …

2693ce3

…setup. (apache#2452) (cherry picked from commit c69955e)

fapifta mentioned this pull request Feb 21, 2023

HDDS-7817. Add HTTP and HTTPS ports to DatanodeDetails #4278

Closed

HDDS-5480. [Ozone-Streaming] Client and server should support stream setup. #2452

HDDS-5480. [Ozone-Streaming] Client and server should support stream setup. #2452

Uh oh!

Conversation

captainzmc commented Jul 22, 2021

What changes were proposed in this pull request?

What is the link to the Apache JIRA

How was this patch tested?

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mukul1987 commented Jul 22, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

captainzmc Jul 27, 2021 • 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!

Choose a reason for hiding this comment

Uh oh!

bshashikant commented Jul 22, 2021

Uh oh!

bshashikant commented Jul 22, 2021

Uh oh!

kaijchen commented Jul 23, 2021

Uh oh!

captainzmc commented Jul 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

captainzmc commented Jul 23, 2021

Uh oh!

szetszwo commented Jul 26, 2021

Uh oh!

captainzmc commented Jul 27, 2021

Uh oh!

szetszwo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

szetszwo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

captainzmc Jul 27, 2021 •

edited

Loading

captainzmc commented Jul 23, 2021 •

edited

Loading