HDDS-11622. Support domain socket creation by ChenSammi · Pull Request #7379 · apache/ozone

ChenSammi · 2024-10-30T09:08:42Z

What changes were proposed in this pull request?

Implemented a factory to create DomainSocket.

What is the link to the Apache JIRA

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

How was this patch tested?

It will be tested in next patch.

jojochuang · 2024-10-31T03:55:28Z

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


+  public static final boolean OZONE_READ_SHORT_CIRCUIT_DEFAULT = false;
+  public static final String OZONE_DOMAIN_SOCKET_PATH = "ozone.domain.socket.path";
+  public static final String OZONE_DOMAIN_SOCKET_PATH_DEFAULT = "/var/lib/ozone/dn_socket";


Can we use existing directories like ozone.metadata.dirs or hdds.metadata.dir

NO. This OZONE_DOMAIN_SOCKET_PATH_DEFAULT file will be created by DomainSocket process. It cannot be an existing file. Also Hadoop native library has different permission requirement on this path.

https://cwiki.apache.org/confluence/display/HADOOP2/SocketPathSecurity

jojochuang · 2024-10-31T03:59:02Z

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

+      type = ConfigType.SIZE,
+      description = "Buffer size of reader/writer.",
+      tags = { ConfigTag.CLIENT, ConfigTag.DATANODE })
+  private int shortCircuitBufferSize = 128 * 1024;


HDFS has default short circuit buffer size of 1MB.

I tested the different values. Bigger buffer size doesn't help to improve the performance.
The short-circuit channel only exchange getBlock request and response. The value is determined by how big will a request and a response of a 256MB block size, 4MB chunk size, 16KB checksum size block. The request is round 500 bytes, and the response is around
30 + 53 * 64 + 6 * 16384 ~ 100k
block size (exclude chunks) - 30 bytes
chunk size (one checksums) - 53 bytes
one checksum size - 6 bytes

jojochuang · 2024-10-31T04:00:24Z

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

+    // See #{java.org.apache.hadoop.net.unix.DomainSocket#validateSocketPathSecurity0}
+    // for details.
+    //
+    // So unless you are running as root or the hdfs superuser, you cannot


Suggested change

// So unless you are running as root or the hdfs superuser, you cannot

// So unless you are running as root or the ozone superuser, you cannot

the ozone superuser -> an ozone admin ?

It means the user who launches the service.

jojochuang · 2024-10-31T04:06:51Z

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

+  public static synchronized DomainSocketFactory getInstance(ConfigurationSource conf) {
+    if (instance == null) {
+      instance = new DomainSocketFactory(conf);
+    }
+    return instance;
+  }


Suggested change

public static synchronized DomainSocketFactory getInstance(ConfigurationSource conf) {

if (instance == null) {

instance = new DomainSocketFactory(conf);

}

return instance;

}

public static DomainSocketFactory getInstance(ConfigurationSource conf) {

if (instance == null) {

synchronized (DomainSocketFactory.class) {

instance = new DomainSocketFactory(conf);

}

}

return instance;

}

Use double-checked locking instead: https://www.baeldung.com/java-singleton-double-checked-locking

ashishkumar50 · 2024-11-04T04:25:25Z

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

+
+  public static final String FEATURE = "short-circuit reads";
+  public static final String FEATURE_FLAG = "SC";
+  private static boolean nativeCodeLoaded = false;


May be better nativeLibraryLoaded instead of nativeCodeLoaded.

make sense.

ashishkumar50 · 2024-11-04T04:28:12Z

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

+    long startTime = System.nanoTime();
+    if (!shortCircuitEnabled) {
+      LOG.info(FEATURE + " is disabled.");
+      pathInfo = PathInfo.NOT_CONFIGURED;


We can ignore putting in pathInfo and further into pathMap when shortCircuit is disabled, we can throw exception?

The DomainSocketFactory.java is an singleton instance. It is only instantiated once. Later reference will call isServiceEnabled and isServiceReady to determine whether DomainSocket can be created or not.

ashishkumar50 · 2024-11-04T04:38:38Z

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

+  }
+
+  public static final String FEATURE = "short-circuit reads";
+  public static final String FEATURE_FLAG = "SC";


Unused FEATURE_FLAG

It will be used in next patch.

jojochuang

Do we plan to add tests around these new code?

We would also need to package libadhoop into Ozone, into Ozone's dev docker image, and add tests for that.

ChenSammi · 2024-11-05T04:43:54Z

Do we plan to add tests around these new code?

We would also need to package libadhoop into Ozone, into Ozone's dev docker image, and add tests for that.

I separated the previous patch into sub patches, for easy review. The separation from an actually complete patch is a little time consuming. The tests will be added in later patches, which require more new logics to run together.

ashishkumar50

Thanks @ChenSammi for updating the patch, LGTM +1.

ChenSammi · 2024-11-06T02:58:04Z

Thanks @jojochuang @smengcl @ashishkumar50 for the review. I will close this JIRA and provide a new one. I ment to push the patch to the feature branch HDDS-10685.

ChenSammi added 2 commits October 30, 2024 16:11

HDDS-11622. Support domain socket creation

2a890b5

checkstyles

c6ce5a9

ChenSammi force-pushed the HDDS-11622 branch from 2fe7a14 to c6ce5a9 Compare October 30, 2024 10:37

findbugs

633bfd9

ChenSammi requested review from ashishkumar50 and jojochuang October 31, 2024 02:46

jojochuang reviewed Oct 31, 2024

View reviewed changes

ChenSammi added 2 commits October 31, 2024 14:21

address comments

c1bf7ab

update the comments

bbca6a6

ashishkumar50 reviewed Nov 4, 2024

View reviewed changes

rename nativeCodeLoaded to nativeLibraryLoaded

d1eb82c

jojochuang reviewed Nov 5, 2024

View reviewed changes

ashishkumar50 reviewed Nov 5, 2024

View reviewed changes

ChenSammi closed this Nov 6, 2024

ashishkumar50 mentioned this pull request Nov 6, 2024

HDDS-11622. Support domain socket creation #7397

Merged

	// So unless you are running as root or the hdfs superuser, you cannot
	// So unless you are running as root or the ozone superuser, you cannot

Conversation

ChenSammi commented Oct 30, 2024

What changes were proposed in this pull request?

What is the link to the Apache JIRA

How was this patch tested?

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!

ChenSammi Oct 31, 2024 • 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!

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!

jojochuang left a comment

Choose a reason for hiding this comment

Uh oh!

ChenSammi commented Nov 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ashishkumar50 left a comment

Choose a reason for hiding this comment

Uh oh!

ChenSammi commented Nov 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ChenSammi Oct 31, 2024 •

edited

Loading

ChenSammi commented Nov 5, 2024 •

edited

Loading

ChenSammi commented Nov 6, 2024 •

edited

Loading