Skip to content

HDDS-9913. Reduce number of times configuration is loaded in Ozone client #5789

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
adoroszlai merged 2 commits into from
Dec 16, 2023
Merged

HDDS-9913. Reduce number of times configuration is loaded in Ozone client #5789

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9ce5a2a
HDDS-9913. EC client Reduces duplicate loading of configurations
xichen01 Dec 13, 2023
db21fd8
Add a StreamBufferArgs for parameter passing
xichen01 Dec 16, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 3 additions & 7 deletions ...ne/client/src/main/java/org/apache/hadoop/ozone/client/io/BlockOutputStreamEntryPool.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@
import java.util.Map;

import org.apache.hadoop.hdds.client.ReplicationConfig;
import org.apache.hadoop.hdds.conf.OzoneConfiguration;
import org.apache.hadoop.hdds.scm.ByteStringConversion;
import org.apache.hadoop.hdds.scm.ContainerClientMetrics;
import org.apache.hadoop.hdds.scm.OzoneClientConfig;
Expand Down Expand Up @@ -124,16 +123,13 @@ ExcludeList createExcludeList() {
Clock.system(ZoneOffset.UTC));
}

BlockOutputStreamEntryPool(ContainerClientMetrics clientMetrics) {
BlockOutputStreamEntryPool(ContainerClientMetrics clientMetrics,
OzoneClientConfig clientConfig) {
streamEntries = new ArrayList<>();
omClient = null;
keyArgs = null;
xceiverClientFactory = null;
config =
new OzoneConfiguration().getObject(OzoneClientConfig.class);
config.setStreamBufferSize(0);
config.setStreamBufferMaxSize(0);
config.setStreamBufferFlushSize(0);
config = clientConfig;
config.setStreamBufferFlushDelay(false);
requestID = null;
int chunkSize = 0;
Expand Down
3 changes: 2 additions & 1 deletion hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/io/ECKeyOutputStream.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,8 @@ public long getFlushCheckpoint() {
}

private ECKeyOutputStream(Builder builder) {
super(builder.getReplicationConfig(), builder.getClientMetrics());
super(builder.getReplicationConfig(), builder.getClientMetrics(),
builder.getClientConfig());
this.config = builder.getClientConfig();
this.bufferPool = builder.getByteBufferPool();
// For EC, cell/chunk size and buffer size can be same for now.
Expand Down
5 changes: 3 additions & 2 deletions hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/io/KeyOutputStream.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,16 +104,17 @@ enum StreamAction {
private boolean atomicKeyCreation;

public KeyOutputStream(ReplicationConfig replicationConfig,
ContainerClientMetrics clientMetrics) {
ContainerClientMetrics clientMetrics, OzoneClientConfig clientConfig) {
this.replication = replicationConfig;
this.config = clientConfig;
closed = false;
this.retryPolicyMap = HddsClientUtils.getExceptionList()
.stream()
.collect(Collectors.toMap(Function.identity(),
e -> RetryPolicies.TRY_ONCE_THEN_FAIL));
retryCount = 0;
offset = 0;
blockOutputStreamEntryPool = new BlockOutputStreamEntryPool(clientMetrics);
blockOutputStreamEntryPool = new BlockOutputStreamEntryPool(clientMetrics, clientConfig);
}

@VisibleForTesting
Expand Down
14 changes: 8 additions & 6 deletions hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,6 +207,7 @@ public class RpcClient implements ClientProtocol {
private final boolean topologyAwareReadEnabled;
private final boolean checkKeyNameEnabled;
private final OzoneClientConfig clientConfig;
private final ReplicationConfigValidator replicationConfigValidator;
private final Cache<URI, KeyProvider> keyProviderCache;
private final boolean getLatestVersionLocation;
private final ByteBufferPool byteBufferPool;
Expand All @@ -230,6 +231,8 @@ public RpcClient(ConfigurationSource conf, String omServiceId)
this.ugi = UserGroupInformation.getCurrentUser();
// Get default acl rights for user and group.
OzoneAclConfig aclConfig = this.conf.getObject(OzoneAclConfig.class);
replicationConfigValidator =
this.conf.getObject(ReplicationConfigValidator.class);
this.userRights = aclConfig.getUserDefaultRights();
this.groupRights = aclConfig.getGroupDefaultRights();

Expand Down Expand Up @@ -1343,9 +1346,7 @@ public OzoneOutputStream createKey(
}

if (replicationConfig != null) {
ReplicationConfigValidator validator =
this.conf.getObject(ReplicationConfigValidator.class);
validator.validate(replicationConfig);
replicationConfigValidator.validate(replicationConfig);
}

OmKeyArgs.Builder builder = new OmKeyArgs.Builder()
Expand Down Expand Up @@ -1854,7 +1855,7 @@ public OzoneDataStreamOutput createMultipartStreamKey(
.setMultipartUploadID(uploadID)
.setIsMultipartKey(true)
.enableUnsafeByteBufferConversion(unsafeByteBufferConversion)
.setConfig(conf.getObject(OzoneClientConfig.class))
.setConfig(clientConfig)
Copy link
Contributor

@adoroszlai adoroszlai Dec 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one, and the other two similar items are creating new configs intentionally. Please see HDDS-8216.

To reduce config loading, but still keep config loading to minimum, we could cache and reuse OzoneClientConfig instance per EC chunk size.

Copy link
Contributor Author

@xichen01 xichen01 Dec 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand, but here we are using Configuration to pass parameters, which shouldn't be the role of Configuration. If a parameter needs to be computed at runtime, it should not be directly obtained from Configuration. Therefore, I added a StreamBufferArgs to modify and pass the StreamBuffe related parameters .

IMO, Configuration should ideally not provide a direct set method. Modifications to Configuration should only occur in "test code" and during "reconfiguration". Otherwise, Configuration should be read-only.

.setAtomicKeyCreation(isS3GRequest.get())
.build();
keyOutputStream
Expand Down Expand Up @@ -2269,7 +2270,7 @@ private OzoneDataStreamOutput createDataStreamOutput(OpenKeySession openKey)
.setOmClient(ozoneManagerClient)
.setReplicationConfig(replicationConfig)
.enableUnsafeByteBufferConversion(unsafeByteBufferConversion)
.setConfig(conf.getObject(OzoneClientConfig.class))
.setConfig(clientConfig)
.setAtomicKeyCreation(isS3GRequest.get())
.build();
keyOutputStream
Expand All @@ -2279,6 +2280,7 @@ private OzoneDataStreamOutput createDataStreamOutput(OpenKeySession openKey)
openKey, keyOutputStream, null);
return new OzoneDataStreamOutput(out != null ? out : keyOutputStream);
}

private OzoneOutputStream createOutputStream(OpenKeySession openKey)
throws IOException {
KeyOutputStream keyOutputStream = createKeyOutputStream(openKey)
Expand Down Expand Up @@ -2351,7 +2353,7 @@ private KeyOutputStream.Builder createKeyOutputStream(
.setXceiverClientManager(xceiverClientManager)
.setOmClient(ozoneManagerClient)
.enableUnsafeByteBufferConversion(unsafeByteBufferConversion)
.setConfig(conf.getObject(OzoneClientConfig.class))
.setConfig(clientConfig)
.setAtomicKeyCreation(isS3GRequest.get())
.setClientMetrics(clientMetrics);
}
Expand Down
4 changes: 3 additions & 1 deletion ...p-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/client/OzoneOutputStreamStub.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@

import org.apache.hadoop.hdds.client.ReplicationConfig;
import org.apache.hadoop.hdds.conf.OzoneConfiguration;
import org.apache.hadoop.hdds.scm.OzoneClientConfig;
import org.apache.hadoop.ozone.client.io.KeyOutputStream;
import org.apache.hadoop.ozone.client.io.OzoneOutputStream;
import org.apache.hadoop.ozone.om.helpers.OmMultipartCommitUploadPartInfo;
Expand Down Expand Up @@ -75,7 +76,8 @@ public synchronized void close() throws IOException {
@Override
public KeyOutputStream getKeyOutputStream() {
return new KeyOutputStream(
ReplicationConfig.getDefault(new OzoneConfiguration()), null) {
ReplicationConfig.getDefault(new OzoneConfiguration()), null,
new OzoneConfiguration().getObject(OzoneClientConfig.class)) {
@Override
public synchronized OmMultipartCommitUploadPartInfo
getCommitUploadPartInfo() {
Expand Down