HDDS-4417. Simplify Ozone client code with configuration object #1542

elek · 2020-11-02T15:05:44Z

What changes were proposed in this pull request?

In HDDS-4185 we agreed to introduce a new configuration for the Ozone client to adjust the behavior of incremental byte buffer.

When I started to add it I realized the code is already very complex as all the required configuration values are propagated manually to the Key/Block/ChunkOutputstream as induvidual constructor parameters.

In this patch I simplify the structure with using only one POJO with configuration annotations.

What is the link to the Apache JIRA

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

How was this patch tested?

With full CI on my fork.

bshashikant · 2020-11-11T06:09:22Z

@elek , can u check the failed checks. The changes in general look good to me. Will look into it in more detail.

elek · 2020-11-11T09:43:44Z

@elek , can u check the failed checks. The changes in general look good to me. Will look into it in more detail.

Thanks to check it. Yes, I am fixing the failures. It was green on my fork, so must be some silly mistake.

bshashikant · 2020-11-12T06:21:53Z

Thanks @elek for the contribution.

adoroszlai

Thanks @elek for this refactoring. I noticed a few issues, can they be addressed in an addendum?

adoroszlai · 2020-11-12T06:39:07Z

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

+  public OzoneClientConfig() {
+  }
+
+  private void validate() {


Currently validate() is unused. I think it should have @PostConstruct annotation.

adoroszlai · 2020-11-12T06:46:40Z

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

+      if (getToken() != null) {
+        UserGroupInformation.getCurrentUser().addToken(getToken());
+      }


This was removed on master in HDDS-4285. I don't think it should have been restored in the merge.

adoroszlai · 2020-11-12T06:49:34Z

hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java

    verifyVolumeName(volumeName);
    verifyBucketName(bucketName);
-    if(checkKeyNameEnabled) {
+    if (clientConfig.isStreamBufferFlushDelay()) {


This condition is not the same as previously.

adoroszlai · 2020-11-12T06:56:01Z

...integration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/TestBlockOutputStream.java

+    OzoneClientConfig config = new OzoneClientConfig();
+    config.setChecksumType(ChecksumType.NONE);
+    conf.setFromObject(config);
+
    conf.setTimeDuration(HDDS_SCM_WATCHER_TIMEOUT, 1000, TimeUnit.MILLISECONDS);
    conf.setTimeDuration(OZONE_SCM_STALENODE_INTERVAL, 3, TimeUnit.SECONDS);
-    conf.set(OzoneConfigKeys.OZONE_CLIENT_CHECKSUM_TYPE, "NONE");
    conf.setQuietMode(false);
    conf.setStorageSize(OzoneConfigKeys.OZONE_SCM_BLOCK_SIZE, 4,
        StorageUnit.MB);
-    conf.setBoolean(OZONE_CLIENT_STREAM_BUFFER_FLUSH_DELAY, false);
+
+    OzoneClientConfig clientConfig = conf.getObject(OzoneClientConfig.class);
+    clientConfig.setStreamBufferFlushDelay(false);
+    conf.setFromObject(clientConfig);
+


Nit: two blocks of OzoneClientConfig can be merged:

OzoneClientConfig clientConfig = conf.getObject(OzoneClientConfig.class); clientConfig.setChecksumType(ChecksumType.NONE); clientConfig.setStreamBufferFlushDelay(false); conf.setFromObject(clientConfig);

adoroszlai · 2020-11-12T06:59:31Z

...ne/integration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/TestOzoneRpcClient.java

  @BeforeClass
  public static void init() throws Exception {
    OzoneConfiguration conf = new OzoneConfiguration();
+    conf.setFromObject(new OzoneClientConfig());


Nit: aren't defaults set automatically without this?

Yes, it's supposed to be set. I was confused by the other pattern used elsewhere:

create OzoneConfig

set custom properties by setter

Here we need to export OzoneClientConfig

modify tags

reset OzoneClientConfig to OzoneConfig

elek · 2020-11-13T08:35:13Z

Thanks the suggestions @adoroszlai Will create a new PR with the same HDDS-4417 tag shortly.

adoroszlai · 2020-11-13T09:25:23Z

Will create a new PR with the same HDDS-4417 tag shortly.

Done: #1581

elek · 2020-11-13T11:48:16Z

Done: #1581

Oh, missed it. Thanks for the fixes.

adoroszlai · 2020-11-18T17:12:48Z

Done: #1581

Oh, missed it. Thanks for the fixes.

It's not yet in, needs review.

* master: (53 commits) HDDS-4458. Fix Max Transaction ID value in OM. (apache#1585) HDDS-4442. Disable the location information of audit logger to reduce overhead (apache#1567) HDDS-4441. Add metrics for ACL related operations.(Addendum for HA). (apache#1584) HDDS-4081. Create ZH translation of StorageContainerManager.md in doc. (apache#1558) HDDS-4080. Create ZH translation of OzoneManager.md in doc. (apache#1541) HDDS-4079. Create ZH translation of Containers.md in doc. (apache#1539) HDDS-4184. Add Features menu for Chinese document. (apache#1547) HDDS-4235. Ozone client FS path validation is not present in OFS. (apache#1582) HDDS-4338. Fix the issue that SCM web UI banner shows "HDFS SCM". (apache#1583) HDDS-4337. Implement RocksDB options cache for new datanode DB utilities. (apache#1544) HDDS-4083. Create ZH translation of Recon.md in doc (apache#1575) HDDS-4453. Replicate closed container for random selected datanodes. (apache#1574) HDDS-4408: terminate Datanode when Datanode State Machine Thread got uncaught exception. (apache#1533) HDDS-4443. Recon: Using Mysql database throws exception and fails startup (apache#1570) HDDS-4315. Use Epoch to generate unique ObjectIDs (apache#1480) HDDS-4455. Fix typo in README.md doc (apache#1578) HDDS-4441. Add metrics for ACL related operations. (apache#1571) HDDS-4437. Avoid unnecessary builder conversion in setting volume Quota/Owner request (apache#1564) HDDS-4417. Simplify Ozone client code with configuration object (apache#1542) HDDS-4363. Add metric to track the number of RocksDB open/close operations. (apache#1530) ...

elek added 9 commits October 29, 2020 14:01

ozone client config

55deff3

fix config handling

e28e737

end

8f7808f

fix

b4dd563

checkstyle fixes

911706c

retrigger ci

d2b553b

finish block output stream

2a75546

fix unit tests

13f3310

HDDS-4417. Simplify Ozone client code with configuration object

44a3aa6

elek changed the title ~~Hdds 4417~~ HDDS-4417. Simplify Ozone client code with configuration object Nov 2, 2020

Merge remote-tracking branch 'origin/master' into HDDS-4417

83a4fe0

elek added 4 commits November 11, 2020 10:46

Restore lines disappeared during the merge

ea7064c

fix findbugs problem

eef29d2

fix copy paste typo

3b7018e

retrigger build

76d62e5

bshashikant approved these changes Nov 12, 2020

View reviewed changes

bshashikant merged commit fdbd5da into apache:master Nov 12, 2020

adoroszlai reviewed Nov 12, 2020

View reviewed changes

adoroszlai mentioned this pull request Nov 12, 2020

HDDS-4417. Simplify Ozone client code with configuration object -- addendum #1581

Merged

HDDS-4417. Simplify Ozone client code with configuration object #1542

HDDS-4417. Simplify Ozone client code with configuration object #1542

Uh oh!

Conversation

elek commented Nov 2, 2020

What changes were proposed in this pull request?

What is the link to the Apache JIRA

How was this patch tested?

Uh oh!

bshashikant commented Nov 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elek commented Nov 11, 2020

Uh oh!

bshashikant commented Nov 12, 2020

Uh oh!

adoroszlai left a comment

Choose a reason for hiding this comment

Uh oh!

adoroszlai Nov 12, 2020

Choose a reason for hiding this comment

Uh oh!

adoroszlai Nov 12, 2020

Choose a reason for hiding this comment

Uh oh!

adoroszlai Nov 12, 2020

Choose a reason for hiding this comment

Uh oh!

adoroszlai Nov 12, 2020

Choose a reason for hiding this comment

Uh oh!

adoroszlai Nov 12, 2020

Choose a reason for hiding this comment

Uh oh!

elek Nov 13, 2020

Choose a reason for hiding this comment

Uh oh!

elek commented Nov 13, 2020

Uh oh!

adoroszlai commented Nov 13, 2020

Uh oh!

elek commented Nov 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adoroszlai commented Nov 18, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bshashikant commented Nov 11, 2020 •

edited

Loading

elek commented Nov 13, 2020 •

edited

Loading