Skip to content

HDDS-6194. EC: Freon ockg support EC write #2996

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
umamaheswararao merged 9 commits into from
Feb 11, 2022
Merged

HDDS-6194. EC: Freon ockg support EC write #2996

Changes from 6 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
bb24b76
freon ockg support EC
captainzmc Jan 18, 2022
58a9c54
ockg: enable bucket-level replication config
kaijchen Feb 10, 2022
a918ce3
fix style
kaijchen Feb 10, 2022
01b5181
ockg: restore --factor option for backward compatibility
kaijchen Feb 10, 2022
8e9793d
fix checkstyle
kaijchen Feb 10, 2022
d9ed09a
ockg: catch NotImplementedException for ECKeyOutputStream#flush()
kaijchen Feb 11, 2022
287a177
Revert "ockg: catch NotImplementedException for ECKeyOutputStream#flu…
kaijchen Feb 11, 2022
ac6aa13
ockg: do not call flush() for EC Keys
kaijchen Feb 11, 2022
abb01da
trigger CI
kaijchen Feb 11, 2022
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
41 changes: 36 additions & 5 deletions hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/freon/OzoneClientKeyGenerator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,10 @@
import java.util.Map;
import java.util.concurrent.Callable;

import org.apache.commons.lang3.NotImplementedException;
import org.apache.hadoop.fs.ozone.OzoneClientUtils;
import org.apache.hadoop.hdds.cli.HddsVersionProvider;
import org.apache.hadoop.hdds.client.ReplicationConfig;
import org.apache.hadoop.hdds.client.ReplicationFactor;
import org.apache.hadoop.hdds.client.ReplicationType;
import org.apache.hadoop.hdds.conf.OzoneConfiguration;
Expand All @@ -30,7 +33,9 @@

import com.codahale.metrics.Timer;
import picocli.CommandLine.Command;
import picocli.CommandLine.Model.CommandSpec;
import picocli.CommandLine.Option;
import picocli.CommandLine.Spec;

/**
* Data generator tool test om performance.
Expand All @@ -44,6 +49,8 @@
public class OzoneClientKeyGenerator extends BaseFreonGenerator
implements Callable<Void> {

@Spec private CommandSpec spec;

@Option(names = {"-v", "--volume"},
description = "Name of the bucket which contains the test data. Will be"
+ " created if missing.",
Expand All @@ -67,22 +74,35 @@ public class OzoneClientKeyGenerator extends BaseFreonGenerator
private int bufferSize;

@Option(names = { "-F", "--factor" },
description = "Replication factor (ONE, THREE)",
description = "[Deprecated] Replication factor (ONE, THREE)",
defaultValue = "THREE"
)
private ReplicationFactor factor = ReplicationFactor.THREE;

@Option(
names = "--om-service-id",
@Option(names = "--om-service-id",
description = "OM Service ID"
)
private String omServiceID = null;
private String omServiceID;

@Option(names = {"--replication"},
description =
"Replication configuration of the new key."
+ "(ONE, THREE) for RATIS or STAND_ALONE, "
+ "(rs-3-2-1024k, rs-6-3-1024k or rs-10-4-1024k) for EC."
)
private String replication;

@Option(names = {"--type"},
description = "Replication type of the new key. (RATIS, STAND_ALONE, EC)"
)
private ReplicationType replicationType;

private Timer timer;

private OzoneBucket bucket;
private ContentGenerator contentGenerator;
private Map<String, String> metadata;
private ReplicationConfig replicationConfig;

@Override
public Void call() throws Exception {
Expand All @@ -94,6 +114,15 @@ public Void call() throws Exception {
contentGenerator = new ContentGenerator(keySize, bufferSize);
metadata = new HashMap<>();

if (spec.commandLine().getParseResult().hasMatchedOption("--factor")) {
replicationConfig = ReplicationConfig
.fromTypeAndFactor(ReplicationType.RATIS, factor);
} else {
replicationConfig = OzoneClientUtils
.validateAndGetClientReplicationConfig(replicationType, replication,
ozoneConfiguration);
}
Comment on lines +117 to +124

@adoroszlai adoroszlai Feb 10, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Something is wrong around option handling. Setting only replication, but neither factor nor type, a regular RATIS/3 key was created:

$ ozone freon ockg -n1 -t1 --replication 'rs-3-2-1024k' -p replication_rs-3-2-1024k
...
Successful executions: 1

$ ozone sh key info /vol1/bucket1/replication_rs-3-2-1024k/0
...
  "replicationConfig" : {
    "replicationFactor" : "THREE",
    "requiredNodes" : 3,
    "replicationType" : "RATIS"
  },
...

@kaijchen kaijchen Feb 10, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It is meaningless to set replication or type only.
And it's also possible to set --type EC with --replication THREE together.

type/replication THREE rs-3-2-1024k
RATIS OK Bad
EC Bad OK

Should we bind this two options together like RATIS/THREE or EC/rs-3-2-1024k instead?

@kaijchen kaijchen Feb 11, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Something is wrong around option handling. Setting only replication, but neither factor nor type, a regular RATIS/3 key was created:

This is due to OzoneClientUtils#validateAndGetClientReplicationConfig, the default client config was used.

ozone/hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/OzoneClientUtils.java

Lines 172 to 175 in c4aec7b

if (userPassedType == null && clientConfiguredDefaultType != null) {
clientReplicationType =
ReplicationType.valueOf(clientConfiguredDefaultType);
}


try (OzoneClient rpcClient = createOzoneClient(omServiceID,
ozoneConfiguration)) {
ensureVolumeAndBucketExist(rpcClient, volumeName, bucketName);
Expand All @@ -112,9 +141,11 @@ private void createKey(long counter) throws Exception {

timer.time(() -> {
try (OutputStream stream = bucket.createKey(key, keySize,
ReplicationType.RATIS, factor, metadata)) {
replicationConfig, metadata)) {
contentGenerator.write(stream);
stream.flush();

@adoroszlai adoroszlai Feb 10, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Got this for EC key:

$ ozone freon ockg -n1 -t1 --replication 'rs-3-2-1024k' --type EC
...
NotImplementedException: The flush API is not implemented yet.
	at org.apache.hadoop.ozone.client.io.ECKeyOutputStream.flush(ECKeyOutputStream.java:468)
	at org.apache.hadoop.ozone.client.io.OzoneOutputStream.flush(OzoneOutputStream.java:55)
	at org.apache.hadoop.ozone.freon.OzoneClientKeyGenerator.lambda$createKey$0(OzoneClientKeyGenerator.java:145)
	at com.codahale.metrics.Timer.time(Timer.java:101)
	at org.apache.hadoop.ozone.freon.OzoneClientKeyGenerator.createKey(OzoneClientKeyGenerator.java:141)

@kaijchen kaijchen Feb 10, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes, this is expected. It should be fixed in another PR.

ozone/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/io/ECKeyOutputStream.java

Lines 466 to 469 in 22e3482

@Override
public void flush() {
throw new NotImplementedException("The flush API is not implemented yet.");
}

@kaijchen kaijchen Feb 11, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should we catch this exception here in ockg?

@umamaheswararao umamaheswararao Feb 11, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Probably for now if Type is EC, ignore to invoke flush? [ We thought not to implement flush unless there is a critical req for it. ]

@kaijchen kaijchen Feb 11, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Probably for now if Type is EC, ignore to invoke flush? [ We thought not to implement flush unless there is a critical req for it. ]

Maybe just catch this exception? Since it's already in a try clause.
And if flush is implemented in the future, it will work without any change.

} catch (NotImplementedException e) {
// flush() is not implemented in ECKeyOutputStream
}
return null;
});
Expand Down