Added Time-Specific Metrics #819

guljain · 2022-07-06T12:37:21Z

Added time-related metrics.
Functions chosen are -

Open : Present in GoogleHadoopFileSystem class with unit test in GoogleHadoopFileSystemIntegrationTest class.
Rename : Presentin GoogleHadoopFileSystem class with unit test in GoogleHadoopFileSystemIntegrationTest class.
Delete : Present in GoogleHadoopFileSystem class with unit test in GoogleHadoopFileSystemIntegrationTest class.
Create : Present in GoogleHadoopFileSystem class with unit test in GoogleHadoopFileSystemIntegrationTest class.
Hsync : Present in GoogleHadoopOutputStream class with unit test in GoogleHadoopOutputStreamTest class.
Hflush : Present in GoogleHadoopOutputStream class with unit test in GoogleHadoopOutputStreamTest class.
Close : Present in GoogleHadoopOutputStream class with unit test in GoogleHadoopOutputStreamTest class.
Seek : Present in GoogleHadoopFSInputStream class with unit test in GoogleHadoopFSInputStreamIntegrationTest class.

gcs/src/main/java/com/google/cloud/hadoop/fs/gcs/GhfsTimeStatistic.java

gcs/src/main/java/com/google/cloud/hadoop/fs/gcs/GoogleHadoopFSInputStream.java

arunkumarchacko · 2022-07-06T14:15:42Z

gcs/src/main/java/com/google/cloud/hadoop/fs/gcs/GhfsTimeStatistic.java

+
+/** Time Statistics which are collected in GCS. */
+@InterfaceStability.Unstable
+public enum GhfsTimeStatistic {


I am not sure creating a mutable enum is the right thing to do. Why not create class instead? Also having this singleton make the testing harder

For the plugin GhfsTimeStatisticPlugin, I had references to loop over various metrics. This is the reason why mutable enum was used. Also by class, do you mean an implementation like https://github.com/LucaCanali/hadoop/blob/s3aAndHDFSTimeInstrumentation/hadoop-tools/hadoop-aws/src/main/java/org/apache/hadoop/fs/s3a/S3ATimeInstrumentation.java ?

I think mutable enums are an anti-pattern since it is sort of like global variables, which are hard to test etc. I was curious why can't we achieve the same functionality we need by using Java class instead of using Java Enum?

Having enum class makes the metrics look more organized in my opinion. I agree with mutable enum issue so I have mapped them with AtomicLong and used it to store the metric's value. Does that seem like a good alternative ?

This is practically a global mutable variable and testing this will be difficult. Hence I do not think this is the right thing to do.

Please open a bug for this and assign to me. Also add a TODO comment in the code with the bug number.

arunkumarchacko · 2022-07-06T14:17:30Z

gcs/src/main/java/com/google/cloud/hadoop/fs/gcs/GoogleHadoopFileSystem.java

@@ -496,14 +496,21 @@ public String getScheme() {

  @Override
  public FSDataInputStream open(Path hadoopPath, int bufferSize) throws IOException {
+    long startTime = System.nanoTime();


why not create a lambda for timing the methods, instead of duplicating the time duration tracking logic?

I am unable to understand on how to use lambda function here. In best case scenario, there will be addition of 2 lines (for startTime and endTime) in each function along with try-finally.

I have shared a sample code with you

Does it mean, need to wrap this entire method call inside lambda? or export existing contents of method to another method and call that from lambda?

Clearly cannot wrap the public method call itself!

arunkumarchacko · 2022-07-07T15:54:59Z

/gcbrun

gcs/src/main/java/com/google/cloud/hadoop/fs/gcs/GhfsTimeStatistic.java

arunkumarchacko · 2022-07-07T16:07:27Z

gcs/src/main/java/com/google/cloud/hadoop/fs/gcs/GoogleHadoopFileSystem.java

@@ -496,14 +496,21 @@ public String getScheme() {

  @Override
  public FSDataInputStream open(Path hadoopPath, int bufferSize) throws IOException {
+    long startTime = System.nanoTime();


I have shared a sample code with you

abmodi · 2022-07-09T02:13:13Z

gcs/src/main/java/com/google/cloud/hadoop/fs/gcs/GhfsTimeStatistic.java

+
+/** Time Statistics capturing total time taken by a function in micro seconds */
+@InterfaceStability.Unstable
+public enum GhfsTimeStatistic {


any plans to capture read/write time?

I believe that is captures in hflush, hsync and close statistics from GoogleHadoopOutputStream. If not, can you point to specific functions ?

arunkumarchacko · 2022-07-18T04:18:07Z

gcs/src/main/java/com/google/cloud/hadoop/fs/gcs/GhfsTimeStatistic.java

+
+/** Time Statistics which are collected in GCS. */
+@InterfaceStability.Unstable
+public enum GhfsTimeStatistic {


This is practically a global mutable variable and testing this will be difficult. Hence I do not think this is the right thing to do.

arunkumarchacko · 2022-07-18T04:19:03Z

gcs/src/test/java/com/google/cloud/hadoop/fs/gcs/GoogleHadoopOutputStreamTest.java

+  public void hsync_increment_timeStatistics() throws IOException {
+    Path objectPath = new Path(ghfs.getUri().resolve("/dir/object2.txt"));
+    FileSystem.Statistics statistics = new FileSystem.Statistics(ghfs.getScheme());
+    GoogleHadoopOutputStream fout =


consider reducing code duplication in this file

Created a single function to test all 3 statistics

gcs/src/main/java/com/google/cloud/hadoop/fs/gcs/GoogleHadoopFSInputStream.java

gcs/src/main/java/com/google/cloud/hadoop/fs/gcs/GoogleHadoopFileSystem.java

medb · 2022-07-20T06:21:15Z

gcs/src/main/java/com/google/cloud/hadoop/fs/gcs/GoogleHadoopFileSystem.java

+      instrumentation.fileCreated();
+      return response;
+    } finally {
+      GhfsTimeStatistic.incrementTimeElapsedMicrosec(


I think that these stats should be FS instance-specific, not global for JVM.

arunkumarchacko · 2022-07-29T04:39:28Z

gcs/src/main/java/com/google/cloud/hadoop/fs/gcs/GhfsStorageStatistics.java

+   *
+   * @return current map of minimums
+   */
+  private Map<String, Long> minimums() {


nit: Please move private methods below public methods.

arunkumarchacko · 2022-07-29T04:51:24Z

gcs/src/main/java/com/google/cloud/hadoop/fs/gcs/GhfsTimeStatistic.java

+
+/** Time Statistics which are collected in GCS. */
+@InterfaceStability.Unstable
+public enum GhfsTimeStatistic {


Please open a bug for this and assign to me. Also add a TODO comment in the code with the bug number.

arunkumarchacko · 2022-08-22T06:38:43Z

/gcbrun

Original PR: GoogleCloudDataproc#819

* Added Time-Specific Metrics Original PR: #819 Co-authored-by: guljain <[email protected]>

arunkumarchacko reviewed Jul 6, 2022

View reviewed changes

arunkumarchacko reviewed Jul 7, 2022

View reviewed changes

abmodi reviewed Jul 9, 2022

View reviewed changes

arunkumarchacko requested changes Jul 18, 2022

View reviewed changes

medb requested changes Jul 20, 2022

View reviewed changes

guljain and others added 2 commits July 22, 2022 10:15

Added Time-Specific Metrics

5db54cd

Merge branch 'master' into Metrics

0a400b2

arunkumarchacko approved these changes Jul 29, 2022

View reviewed changes

guljain added 5 commits July 29, 2022 11:16

Update GhfsTimeStatistic.java

902e7d1

Update GhfsStorageStatistics.java

54b1081

Merge branch 'master' into Metrics

2a30362

Update GhfsTimeStatistic.java

eb513a7

Update GhfsTimeStatistic.java

d557eae

arunkumarchacko pushed a commit to arunkumarchacko/hadoop-connectors that referenced this pull request Sep 5, 2022

Added Time-Specific Metrics

986df6b

Original PR: GoogleCloudDataproc#819

arunkumarchacko mentioned this pull request Sep 5, 2022

Added Time-Specific Metrics #867

Merged

arunkumarchacko added a commit that referenced this pull request Sep 22, 2022

Add Time-Specific Metrics (#867)

a820950

* Added Time-Specific Metrics Original PR: #819 Co-authored-by: guljain <[email protected]>

arunkumarchacko closed this Sep 22, 2022

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added Time-Specific Metrics #819

Added Time-Specific Metrics #819

guljain commented Jul 6, 2022 •

edited

Loading

arunkumarchacko Jul 6, 2022

guljain Jul 6, 2022

arunkumarchacko Jul 7, 2022

guljain Jul 14, 2022

arunkumarchacko Jul 18, 2022

arunkumarchacko Jul 29, 2022

arunkumarchacko Jul 6, 2022

guljain Jul 6, 2022

arunkumarchacko Jul 7, 2022

vinayakumarb Jul 12, 2022

arunkumarchacko commented Jul 7, 2022

arunkumarchacko Jul 7, 2022

abmodi Jul 9, 2022

guljain Jul 11, 2022

arunkumarchacko Jul 18, 2022

arunkumarchacko Jul 18, 2022

guljain Jul 22, 2022

medb Jul 20, 2022

arunkumarchacko Jul 29, 2022

arunkumarchacko Jul 29, 2022

arunkumarchacko commented Aug 22, 2022

Added Time-Specific Metrics #819

Added Time-Specific Metrics #819

Conversation

guljain commented Jul 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arunkumarchacko commented Jul 7, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arunkumarchacko commented Aug 22, 2022

guljain commented Jul 6, 2022 •

edited

Loading