HDDS-5212. Create implementation of OmTransport which uses Grpc #2580

neils-dev · 2021-08-26T01:19:19Z

What changes were proposed in this pull request?

S3 gateway to issue s3 commands through OzoneManagerProtocol over gRPC OmTransport. This PR includes the implementation of the gRPC OmTransport provided by GrpcOmTransport created through a GrpcOmTransportFactory.

The GrpcOmTransport provides a persistent channel connection between the s3gateway and the ozone manager. The port used for the connection (8981) is configurable through a defined configuration class. Similarly, the host is configurable through configuration file through OZONE_OM_ADDRESS_KEY.

In addition, the connection is persistent and recoverable though internal gRPC channel implementation retry as recommended by gRPC dev community.

What is the link to the Apache JIRA

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

How was this patch tested?

Patch tested with new unit tests testing OmTransport Grpc service provider selection, OmTransport Grpc Factory creating GrpcOmTransport gRPC s3g om client for processing s3 requests through OzoneManagerProtocol.

i.) hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/protocolPB/TestGrpcOmTransport.java
Tests gRPC selection for rpc, starting and stopping s3g gRPC client, GrpcOmTransport through GrpcOmTransportFactory.

ii.) hadoop-ozone/common/src/test/java/org/apache/hadoop/ozone/om/protocolPB/TestS3GrpcOmTransport.java
Tests gRPC client server messaging channel by configuring GrpcOmTransport channel connection, sending OMRequest through channel and receiving OMResponse from mock OzoneManagerServiceGrpc server.

$ cd hadoop-ozone/s3gateway
$ mvn -Dtest=TestGrpcOmTransport test

[INFO] -------------------------------------------------------
[INFO]  T E S T S
[INFO] -------------------------------------------------------
[INFO] Running org.apache.hadoop.ozone.protocolPB.TestGrpcOmTransport
[INFO] Tests run: 2, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.488 s - in org.apache.hadoop.ozone.protocolPB.TestGrpcOmTransport
[INFO] 
[INFO] Results:
[INFO] 
[INFO] Tests run: 2, Failures: 0, Errors: 0, Skipped: 0
[INFO] 
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  8.321 s

$ cd hadoop-ozone/common
$ mvn -Dtest=TestS3GrpcOmTransport test

[INFO] -------------------------------------------------------
[INFO]  T E S T S
[INFO] -------------------------------------------------------
[INFO] Running org.apache.hadoop.ozone.om.protocolPB.TestS3GrpcOmTransport
[INFO] Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 1.076 s - in org.apache.hadoop.ozone.om.protocolPB.TestS3GrpcOmTransport
[INFO] 
[INFO] Results:
[INFO] 
[INFO] Tests run: 1, Failures: 0, Errors: 0, Skipped: 0
[INFO] 
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  7.438 s

bharatviswa504 · 2021-08-27T12:03:13Z

hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/protocolPB/GrpcOmTransport.java

+  public GrpcOmTransport(ConfigurationSource conf,
+                          UserGroupInformation ugi, String omServiceId)
+      throws IOException {
+    Optional<String> omHost = getHostNameFromConfigKeys(conf,


In HA we have 3 OMS, we considered only one OM here

And also failover to find leader logic I don't see

Thanks @bharatviswa504 for reviewing. Thanks for your comments. HA feature support is upcoming in subsequent PR assoc with HDDS-5544. There we will support multiple OMs (3) for both grpc client and server with hosts and ports obtained respectively from conf through OZONE_OM_ADDRESS_KEY and OZONE_OM_GRPC_PORT_KEY uniquely identified by serviceid (federation N/A) and nodeid. The logic for failover (simplified) is also found in upcoming HDDS-5544. Let me know if you would like to see something added here related to HA support prior to it.

hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/protocolPB/GrpcOmTransport.java

bharatviswa504 · 2021-08-27T12:05:56Z

hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/protocolPB/GrpcOmTransport.java

+    NettyChannelBuilder channelBuilder =
+        NettyChannelBuilder.forAddress(host, port)
+            .usePlaintext()
+            .maxInboundMessageSize(OzoneConsts.OZONE_SCM_CHUNK_MAX_SIZE);


Max Inbound message should be large value I believe, list output can be large.
Should this have own config like how hadoop rpc has?

Added ozone.om.grpc.maximum.response.length to om config keys similar to hadoop rpc ipc.maximum.response.length with default size 128 * 1024 * 1024. Used in channel configuration for maxInboundMessageSize. In updated commit and push.

…ys for s3g om OmRequest OmResponses over gRPC.

kerneltime · 2021-08-28T15:22:21Z

hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/protocolPB/GrpcOmTransport.java

+    try {
+      channel.awaitTermination(5, TimeUnit.SECONDS);
+    } catch (Exception e) {
+      LOG.error("failed to shutdown OzoneManagerServiceGrpc channel", e);


Does this need to be an error log?

Thanks @kerneltime for reviewing. Logged here at error level to reflect log at exception level (log indicative of raised exception). Can update if not of value.

kerneltime · 2021-08-28T15:44:22Z

...-ozone/common/src/test/java/org/apache/hadoop/ozone/om/protocolPB/TestS3GrpcOmTransport.java

+  private final OMResponse omResponse = OMResponse.newBuilder()
+                  .setSuccess(true)


Minor Nit: Good to set custom values that are checked.

Suggested change

private final OMResponse omResponse = OMResponse.newBuilder()

.setSuccess(true)

private final OMResponse omResponse = OMResponse.newBuilder()

.setLeaderOMNodeId("testOM")

.setSuccess(true)

Added custom value as noted to latest commit. Same as suggested, using a constant to reflect OMResponse leaderOMNodeId.

private final String leaderOMNodeId = "TestOM";

kerneltime · 2021-08-28T15:44:58Z

...-ozone/common/src/test/java/org/apache/hadoop/ozone/om/protocolPB/TestS3GrpcOmTransport.java

+    Assert.assertEquals(resp.getStatus(), OK);
+  }


Minor Nit: Good to set custom values that are checked.

Suggested change

Assert.assertEquals(resp.getStatus(), OK);

}

Assert.assertEquals(resp.getStatus(), OK);

Assert.assertEquals(resp.getLeaderOMNodeId(),"testOM");

}

Added custom value as noted to latest commit. Same as suggested, using a constant to reflect OMResponse leaderOMNodeId.

private final String leaderOMNodeId = "TestOM";

kerneltime · 2021-08-28T15:45:43Z

...-ozone/common/src/test/java/org/apache/hadoop/ozone/om/protocolPB/TestS3GrpcOmTransport.java

+                                              .OzoneManagerProtocolProtos
+                                              .OMResponse>
+                                          responseObserver) {
+                  LOG.info("GrpcOzoneManagerServer: "


Nit: Unclear why we need to log this and why at info

Removed unnecessary log at info level. Update in latest commit.

kerneltime · 2021-08-28T15:47:51Z

...op-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/protocolPB/TestGrpcOmTransport.java

+  private static final Logger LOG =
+      LoggerFactory.getLogger(TestGrpcOmTransport.class);
+  @Rule
+  public Timeout timeout = Timeout.seconds(300);


Nit: This test should never take more than 30 seconds.

Updated timeout to 30s in latest commit.

kerneltime

Some minor review comments. LGTM

… test timeout to appropriate test timeout value from 5min to 30s and using custom values (to reflect server response) in OMResponse to validate gRPC transport of Ozone Manager Protocol.

neils-dev · 2021-08-31T01:07:37Z

Thanks @kerneltime for your comments. Latest push has updates as noted.

…m.response.length.

bharatviswa504

+1 LGTM

kerneltime · 2021-09-09T16:47:41Z

@neils-dev can you please resolve the conflicting files.

neils-dev · 2021-09-10T15:03:01Z

Thanks @kerneltime for your comments. Latest push has updates as noted.

@neils-dev can you please resolve the conflicting files.

@kerneltime - should be resolved with latest commit. Thanks. Let me know if anything is missing.

swagle · 2021-09-15T14:47:38Z

Thanks @bharatviswa504 and @kerneltime for reviews, merging this.

neils-dev added 2 commits August 25, 2021 15:05

Initial commit with s3 gRPC client and associated unit tests.

2b3395a

Trigger Build

b92a9b5

swagle requested a review from bharatviswa504 August 26, 2021 14:07

bharatviswa504 reviewed Aug 27, 2021

View reviewed changes

hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/protocolPB/GrpcOmTransport.java Show resolved Hide resolved

bharatviswa504 reviewed Aug 27, 2021

View reviewed changes

Added configurable OZONE_OM_GRPC_MAXIMUM_RESPONSE_LENGTH to config ke…

ecef826

…ys for s3g om OmRequest OmResponses over gRPC.

kerneltime reviewed Aug 28, 2021

View reviewed changes

kerneltime approved these changes Aug 28, 2021

View reviewed changes

Added suggested changes to unit tests from review. Including updating…

7913bb8

… test timeout to appropriate test timeout value from 5min to 30s and using custom values (to reflect server response) in OMResponse to validate gRPC transport of Ozone Manager Protocol.

Added configuration setting in ozone-default for ozone.om.grpc.maximu…

8845229

…m.response.length.

bharatviswa504 approved these changes Sep 9, 2021

View reviewed changes

Rebased with master feature branch to fix merge conflicts.

daf208f

swagle merged commit a115ebf into apache:HDDS-4440-s3-performance Sep 15, 2021

neils-dev mentioned this pull request Apr 13, 2022

HDDS-4440. HDDS-4440-s3-performance branch Merge to Master PR - draft #3297

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HDDS-5212. Create implementation of OmTransport which uses Grpc #2580

HDDS-5212. Create implementation of OmTransport which uses Grpc #2580

neils-dev commented Aug 26, 2021

bharatviswa504 Aug 27, 2021

bharatviswa504 Aug 27, 2021

neils-dev Aug 27, 2021

bharatviswa504 Aug 27, 2021

neils-dev Aug 27, 2021

kerneltime Aug 28, 2021

neils-dev Aug 31, 2021

kerneltime Aug 28, 2021

neils-dev Aug 31, 2021

kerneltime Aug 28, 2021 •

edited

neils-dev Aug 31, 2021

kerneltime Aug 28, 2021

neils-dev Aug 31, 2021

kerneltime Aug 28, 2021

neils-dev Aug 31, 2021

kerneltime left a comment

neils-dev commented Aug 31, 2021

bharatviswa504 left a comment

kerneltime commented Sep 9, 2021

neils-dev commented Sep 10, 2021

swagle commented Sep 15, 2021

		private final OMResponse omResponse = OMResponse.newBuilder()
		.setSuccess(true)

HDDS-5212. Create implementation of OmTransport which uses Grpc #2580

HDDS-5212. Create implementation of OmTransport which uses Grpc #2580

Conversation

neils-dev commented Aug 26, 2021

What changes were proposed in this pull request?

What is the link to the Apache JIRA

How was this patch tested?

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kerneltime Aug 28, 2021 • edited

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

kerneltime left a comment

Choose a reason for hiding this comment

neils-dev commented Aug 31, 2021

bharatviswa504 left a comment

Choose a reason for hiding this comment

kerneltime commented Sep 9, 2021

neils-dev commented Sep 10, 2021

swagle commented Sep 15, 2021

kerneltime Aug 28, 2021 •

edited