HDDS-6455. Adding a Builder class to Freon ContentGenerator, so as to avoid Constructor Telescoping. #3242

ArafatKhan2198 · 2022-03-25T13:47:53Z

What changes were proposed in this pull request?

The ContentGenerator class of the freon suite has multiple Parametrised Constructors each having various different arguments that have to be initialised, this affects the readability of the code. One way to solve this is to incorporate a builder class for ContentGenerator class so that any new parameters added in the future can be seamlessly added without the need of creating a new constructor.

When a file was closed or flushed, the file contents were written from the DataNode into the operating system using the normal close() and write() system calls.

The data may not be immediately persisted to the underlying physical storage, but may still reside in-memory in the operating system's file cache. This creates a window of vulnerability where if multiple DataNode machines fail simultaneously (e.g. loss of power to a rack), then previously written data may be lost. To combat this problem, HDFS (as of Hadoop 2.0) has introduced new APIs to provide a way to guarantee that written data will be immediately persisted to the underlying physical storage. These APIs are described in the following table.

hflush() : This Method present inside the FSDataOutputStream.java flushes all outstanding data (i.e. the current unfinished packet) from the client into the OS buffers on all DataNode replicas.

hsync() : This Method present inside the FSDataOutputStream.java flushes the data to the DataNodes, like hflush(), but should also force the data to underlying physical storage via fsync (or equivalent).

Hence I have added a method that would implement Hsync or Hflush after every write if the user wants !

What is the link to the Apache JIRA

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

How was this patch tested?

Existing UTs for ContentGeneratorClass ran successfully

… avoid Constructor Telescoping.

hadoop-ozone/tools/src/test/java/org/apache/hadoop/ozone/freon/TestContentGenerator.java

hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/freon/HadoopDirTreeGenerator.java

hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/freon/ContentGenerator.java

kaijchen · 2022-03-25T15:23:43Z

Thanks for the patch @ArafatKhan2198, I have left some comments for you to check.

ArafatKhan2198 · 2022-04-09T11:48:46Z

Thanks for the patch @ArafatKhan2198, I have left some comments for you to check.

@kaijchen all changes done !!

hadoop-ozone/tools/src/test/java/org/apache/hadoop/ozone/freon/TestContentGenerator.java

hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/freon/HadoopDirTreeGenerator.java

hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/freon/ContentGenerator.java

adoroszlai · 2022-04-19T06:40:07Z

hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/freon/ContentGenerator.java

+      ContentGenerator contentgenerator = new ContentGenerator(this);
+      return contentgenerator;


Builder's build method should verify that the object being constructed is valid. Here hflush and hsync are not applicable at the same time. Please add a check for that.

Nit: no need for local variable, return directly.

Suggested change

ContentGenerator contentgenerator = new ContentGenerator(this);

return contentgenerator;

return new ContentGenerator(this);

adoroszlai · 2022-04-19T06:46:53Z

hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/freon/HadoopFsGenerator.java


    contentGenerator =
-        new ContentGenerator(fileSize, bufferSize, copyBufferSize);
+        new Builder()


Nit: I think it's more readable if builder is qualified where it is being used (and also more consistent with usage in other generator classes).

Suggested change

new Builder()

new ContentGenerator.Builder()

adoroszlai · 2022-04-19T06:46:59Z

hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/freon/HadoopFsGenerator.java

 import org.apache.hadoop.hdds.conf.OzoneConfiguration;

 import com.codahale.metrics.Timer;
+import org.apache.hadoop.ozone.freon.ContentGenerator.Builder;


Suggested change

import org.apache.hadoop.ozone.freon.ContentGenerator.Builder;

adoroszlai

Thanks @ArafatKhan2198 for updating the PR with the enum from #3192.

adoroszlai · 2022-04-19T11:21:15Z

hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/freon/ContentGenerator.java

+    public Builder setFlushMode(String flushmode) {
+      this.flushMode = flushmode;
+      Flags type = Flags.valueOf(flushmode);


I think it would be better to pass an instance of the enum to this method, enforcing type safety.

adoroszlai · 2022-04-19T11:23:12Z

hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/freon/ContentGenerator.java

+  /**
+   * Issue Hsync after every write ( Cannot be used with Hflush ).
+   */
+  private final boolean hSync;
+
+  /**
+   * Issue Hflush after every write ( Cannot be used with Hsync ).
+   */
+  private final boolean hFlush;


Replacing these with a single instance of Flags would simplify the code.

adoroszlai · 2022-04-19T11:23:31Z

hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/freon/ContentGenerator.java

+  /**
+   * Type of flags.
+   */
+  public enum Flags {


I think FlushMode would be a better name.

adoroszlai · 2022-04-19T11:24:22Z

hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/freon/ContentGenerator.java

+    hSync,
+    hFlush,
+    None,


Constants are usually preferred in CAPS.

adoroszlai · 2022-07-03T12:49:05Z

/pending

github-actions

Marking this issue as un-mergeable as requested.

Please use /ready comment when it's resolved.

Please note that the PR will be closed after 21 days of inactivity from now. (But can be re-opened anytime later...)

/pending

github-actions · 2022-07-25T00:17:28Z

Thank you very much for the patch. I am closing this PR temporarily as there was no activity recently and it is waiting for response from its author.

It doesn't mean that this PR is not important or ignored: feel free to reopen the PR at any time.

It only means that attention of committers is not required. We prefer to keep the review queue clean. This ensures PRs in need of review are more visible, which results in faster feedback for all PRs.

If you need ANY help to finish this PR, please contact the community on the mailing list or the slack channel."

Mohammad Arafat Khan added 8 commits March 16, 2022 11:34

Added the builder class to ContentGenerator

76c17f3

Making changes for testing

da1099a

Files changed

8584333

Changes Implemented

556b73f

Made changes to the builder class

39bf3ab

Made minor changes

3fcdef4

HDDS-6455. Adding a Builder class to Freon ContentGenerator, so as to…

30a8c97

… avoid Constructor Telescoping.

Fixed CheckStyle Issues

2e49fb1

kaijchen reviewed Mar 25, 2022

View reviewed changes

hadoop-ozone/tools/src/test/java/org/apache/hadoop/ozone/freon/TestContentGenerator.java Outdated Show resolved Hide resolved

kaijchen reviewed Mar 25, 2022

View reviewed changes

hadoop-ozone/tools/src/test/java/org/apache/hadoop/ozone/freon/TestContentGenerator.java Outdated Show resolved Hide resolved

kaijchen reviewed Mar 25, 2022

View reviewed changes

hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/freon/HadoopDirTreeGenerator.java Outdated Show resolved Hide resolved

kaijchen reviewed Mar 25, 2022

View reviewed changes

hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/freon/ContentGenerator.java Outdated Show resolved Hide resolved

kaijchen reviewed Mar 25, 2022

View reviewed changes

hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/freon/ContentGenerator.java Show resolved Hide resolved

Mohammad Arafat Khan added 4 commits March 25, 2022 20:59

Changed the method names

72e6ffd

Removed Redundant new lines

4c4fc52

Fixed Style Problems

6234165

Fixed Style Problems

4521096

jojochuang reviewed Apr 13, 2022

View reviewed changes

hadoop-ozone/tools/src/test/java/org/apache/hadoop/ozone/freon/TestContentGenerator.java Outdated Show resolved Hide resolved

jojochuang reviewed Apr 13, 2022

View reviewed changes

hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/freon/HadoopDirTreeGenerator.java Outdated Show resolved Hide resolved

jojochuang reviewed Apr 18, 2022

View reviewed changes

hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/freon/ContentGenerator.java Outdated Show resolved Hide resolved

Fixed styling issues

f9682bc

ArafatKhan2198 requested a review from jojochuang April 18, 2022 12:52

Update HadoopDirTreeGenerator.java

6629a87

jojochuang approved these changes Apr 19, 2022

View reviewed changes

adoroszlai reviewed Apr 19, 2022

View reviewed changes

Replaced boolean flags with Enums.

37f2dea

ArafatKhan2198 requested a review from adoroszlai April 19, 2022 09:59

adoroszlai reviewed Apr 19, 2022

View reviewed changes

github-actions bot added the pending label Jul 3, 2022

github-actions bot requested changes Jul 3, 2022

View reviewed changes

github-actions bot closed this Jul 25, 2022

		ContentGenerator contentgenerator = new ContentGenerator(this);
		return contentgenerator;

	ContentGenerator contentgenerator = new ContentGenerator(this);
	return contentgenerator;
	return new ContentGenerator(this);

HDDS-6455. Adding a Builder class to Freon ContentGenerator, so as to avoid Constructor Telescoping. #3242

HDDS-6455. Adding a Builder class to Freon ContentGenerator, so as to avoid Constructor Telescoping. #3242

Uh oh!

Conversation

ArafatKhan2198 commented Mar 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

What is the link to the Apache JIRA

How was this patch tested?

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kaijchen commented Mar 25, 2022

Uh oh!

ArafatKhan2198 commented Apr 9, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

adoroszlai Apr 19, 2022

Choose a reason for hiding this comment

Uh oh!

adoroszlai Apr 19, 2022

Choose a reason for hiding this comment

Uh oh!

adoroszlai Apr 19, 2022

Choose a reason for hiding this comment

Uh oh!

adoroszlai left a comment

Choose a reason for hiding this comment

Uh oh!

adoroszlai Apr 19, 2022

Choose a reason for hiding this comment

Uh oh!

adoroszlai Apr 19, 2022

Choose a reason for hiding this comment

Uh oh!

adoroszlai Apr 19, 2022

Choose a reason for hiding this comment

Uh oh!

adoroszlai Apr 19, 2022

Choose a reason for hiding this comment

Uh oh!

adoroszlai commented Jul 3, 2022

Uh oh!

github-actions bot left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jul 25, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ArafatKhan2198 commented Mar 25, 2022 •

edited

Loading