HDDS-11627. Support getBlock operation on short-circuit channel. #7456

ChenSammi · 2024-11-19T15:06:29Z

What changes were proposed in this pull request?

support DomainSocket server in Datanode
support new client for DomainSocket
provide the getBlock operation on DomainSocket channel
read data from InputStream passed through DomainSocket
add DomainSocket related metrics in Datanode

What is the link to the Apache JIRA

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

How was this patch tested?

new unit tests and integration tests.

ChenSammi · 2024-11-19T15:08:48Z

Current CI build will fail due to new DomainSocket#close(boolean) API is not available in Hadoop common 3.3.6 jar.

smengcl · 2024-11-19T16:05:09Z

hadoop-ozone/integration-test/libhadoop.so

Is it a good idea to include binary like this?

Does the binary come from a released hadoop version? If so, should we write a script snippet to download the tarball and extract the binary from that instead?

This is only for integration test, like the ozone-site.xml files under many test/resources folders. It will not be used in the final dist package and deployment.

I get it. Still, this is a bit hacky IMO :)

Can we at least document where you get/how you build the binaries?

smengcl

Thanks @ChenSammi for the patch.

I've only looked at test cases and some proto changes at this point. Here are my comments so far.

smengcl · 2024-11-20T17:51:40Z

hadoop-hdds/common/src/test/java/org/apache/hadoop/hdds/utils/TestProtobufUtils.java

+    long bcsid = 3L;
+    String datanodeID = UUID.randomUUID().toString();
+    ContainerProtos.DatanodeBlockID.Builder blkIDBuilder =
+        ContainerProtos.DatanodeBlockID.newBuilder().setContainerID(containerID)


nit

Suggested change

ContainerProtos.DatanodeBlockID.newBuilder().setContainerID(containerID)

ContainerProtos.DatanodeBlockID.newBuilder()

.setContainerID(containerID)

containerID is repeated in the base ContainerCommandRequest below as well.

This maybe unrelated to this jira, but can we get rid of this duplication?

smengcl · 2024-11-20T17:55:20Z

hadoop-hdds/common/src/test/java/org/apache/hadoop/hdds/utils/TestProtobufUtils.java

+    request = ContainerProtos.ContainerCommandRequestProto.parseFrom(requestInBytes);
+    assertTrue(request.hasGetBlock());
+    assertEquals(ContainerProtos.Type.GetBlock, request.getCmdType());
+    assertEquals(containerID, request.getContainerID());


Since we still have this duplicated field, we should check both

Suggested change

assertEquals(containerID, request.getContainerID());

assertEquals(containerID, request.getContainerID());

assertEquals(containerID, request.getGetBlock().getBlockID().getContainerID());

smengcl · 2024-11-20T18:02:52Z

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

@@ -287,6 +287,7 @@ enum ReplicationType {
    STAND_ALONE = 2;
    CHAINED = 3;
    EC = 4;
+    SHORT_CIRCUIT = 5;


Hmm, why is SHORT_CIRCUIT a ReplicationType? This looks hacky to me

smengcl · 2024-11-20T18:06:43Z

...zone/integration-test/src/test/java/org/apache/hadoop/hdds/scm/TestXceiverClientManager.java

      XceiverClientSpi client1 = clientManager
-          .acquireClient(container1.getPipeline());
+          .acquireClientForReadData(container1.getPipeline(), true);


Shall we parameterize this? Both allowShortCircuit = {true, false} should be tested IMO

smengcl · 2024-11-20T18:22:43Z

...ne/integration-test/src/test/java/org/apache/hadoop/hdds/scm/TestXceiverClientManagerSC.java

+      XceiverClientSpi client2 = clientManager.acquireClientForReadData(container1.getPipeline(), true);
+      assertTrue(client2 instanceof XceiverClientShortCircuit);


Is this possible to get a non-SCR client (client3) on container1 pipeline at this point?
If so, can we also add a few lines in this test case for that? e.g.:

XceiverClientSpi client3 = clientManager.acquireClientForReadData(container1.getPipeline(), false); assertTrue(client3 instanceof XceiverClientRatis);

smengcl · 2024-11-20T18:49:59Z

...integration-test/src/test/java/org/apache/hadoop/hdds/scm/TestXceiverServerDomainSocket.java

+  @Test
+  public void testReadWrite1() throws IOException {


Does this test case need a timeout? Use class-wide timeout if so.

smengcl · 2024-11-20T18:52:37Z

...integration-test/src/test/java/org/apache/hadoop/hdds/scm/TestXceiverServerDomainSocket.java

+  /**
+   * Test a successful connection and then read/write.
+   */
+  @Test
+  public void testReadWrite1() throws IOException {
+    testReadWrite(false, false);
+  }
+
+  /**
+  *  On Linux, when there is still open file handle of a deleted file, the file handle remains open and can still
+   * be used to read and write the file.
+   */
+  @Test
+  @Timeout(30)
+  public void testReadWrite2() throws IOException {
+    testReadWrite(true, false);
+  }
+
+  @Test
+  @Timeout(30)
+  public void testReadWrite3() throws IOException {
+    testReadWrite(false, true);
+  }
+
+  private void testReadWrite(boolean deleteFileBeforeRead, boolean deleteFileDuringRead) throws IOException {


Consider using @ParameterizedTest instead like:

ozone/hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/upgrade/TestSCMHAUnfinalizedStateValidationAction.java

Lines 65 to 72 in d602eec

@ParameterizedTest

@CsvSource({

"true, true",

"true, false",

"false, true",

"false, false",

})

public void testUpgrade(boolean haEnabledBefore,

smengcl · 2024-11-20T19:06:15Z

...integration-test/src/test/java/org/apache/hadoop/hdds/scm/TestXceiverServerDomainSocket.java

+  @Timeout(30)
+  public void testReadTimeout() throws InterruptedException {
+    conf.set(OzoneClientConfig.OZONE_DOMAIN_SOCKET_PATH, new File(dir, "ozone-socket").getAbsolutePath());
+    conf.set(OzoneConfigKeys.OZONE_CLIENT_READ_TIMEOUT, "5s");


Can we decrease this to 2s to reduce sleep() time without introducing flakiness?

smengcl · 2024-11-20T19:08:07Z

...integration-test/src/test/java/org/apache/hadoop/hdds/scm/TestXceiverServerDomainSocket.java

+  public void testMaxXceiverCount() throws IOException, InterruptedException {
+    conf.set(OzoneClientConfig.OZONE_DOMAIN_SOCKET_PATH, new File(dir, "ozone-socket").getAbsolutePath());


OzoneConfigKeys.OZONE_CLIENT_READ_TIMEOUT is not explicitly set in this test case.

Should set it here as well?

smengcl · 2024-11-20T19:18:26Z

...tion-test/src/test/java/org/apache/hadoop/ozone/client/rpc/read/TestDomainSocketFactory.java

+    try {
+      // temporary disable short-circuit read
+      long pathExpireDuration = factory.getPathExpireMills();
+      factory.disableShortCircuit();


disableShortCircuit() disables short circuit for a short while, and schedules it to be enabled after a set period of time.

Can you help me understand why this is useful?

smengcl · 2024-11-20T20:50:19Z

hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/DatanodeVersion.java

@@ -34,6 +34,8 @@ public enum DatanodeVersion implements ComponentVersion {
  COMBINED_PUTBLOCK_WRITECHUNK_RPC(2, "WriteChunk can optionally support " +
          "a PutBlock request"),

+  SHORT_CIRCUIT_READS(3, "Support short-circuit reads."),


Suggested change

SHORT_CIRCUIT_READS(3, "Support short-circuit reads."),

SHORT_CIRCUIT_READS(3, "Version with short-circuit read support."),

HDDS-11627. Support getBlock operation on short-circuit channel.

e04c83b

ChenSammi marked this pull request as draft November 19, 2024 15:07

smengcl reviewed Nov 19, 2024

View reviewed changes

smengcl reviewed Nov 20, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HDDS-11627. Support getBlock operation on short-circuit channel. #7456

HDDS-11627. Support getBlock operation on short-circuit channel. #7456

ChenSammi commented Nov 19, 2024 •

edited

Loading

ChenSammi commented Nov 19, 2024

smengcl Nov 19, 2024

ChenSammi Nov 20, 2024 •

edited

Loading

smengcl Nov 20, 2024

smengcl left a comment

smengcl Nov 20, 2024

smengcl Nov 20, 2024

smengcl Nov 20, 2024

smengcl Nov 20, 2024

smengcl Nov 20, 2024

smengcl Nov 20, 2024

smengcl Nov 20, 2024

smengcl Nov 20, 2024

smengcl Nov 20, 2024

smengcl Nov 20, 2024

smengcl Nov 20, 2024

smengcl Nov 20, 2024

	ContainerProtos.DatanodeBlockID.newBuilder().setContainerID(containerID)
	ContainerProtos.DatanodeBlockID.newBuilder()
	.setContainerID(containerID)

	assertEquals(containerID, request.getContainerID());
	assertEquals(containerID, request.getContainerID());
	assertEquals(containerID, request.getGetBlock().getBlockID().getContainerID());

		XceiverClientSpi client2 = clientManager.acquireClientForReadData(container1.getPipeline(), true);
		assertTrue(client2 instanceof XceiverClientShortCircuit);

	@ParameterizedTest
	@CsvSource({
	"true, true",
	"true, false",
	"false, true",
	"false, false",
	})
	public void testUpgrade(boolean haEnabledBefore,

		public void testMaxXceiverCount() throws IOException, InterruptedException {
		conf.set(OzoneClientConfig.OZONE_DOMAIN_SOCKET_PATH, new File(dir, "ozone-socket").getAbsolutePath());

	SHORT_CIRCUIT_READS(3, "Support short-circuit reads."),
	SHORT_CIRCUIT_READS(3, "Version with short-circuit read support."),

HDDS-11627. Support getBlock operation on short-circuit channel. #7456

Are you sure you want to change the base?

HDDS-11627. Support getBlock operation on short-circuit channel. #7456

Conversation

ChenSammi commented Nov 19, 2024 • edited Loading

What changes were proposed in this pull request?

What is the link to the Apache JIRA

How was this patch tested?

ChenSammi commented Nov 19, 2024

Choose a reason for hiding this comment

ChenSammi Nov 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smengcl 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

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

ChenSammi commented Nov 19, 2024 •

edited

Loading

ChenSammi Nov 20, 2024 •

edited

Loading