Track latency, retry and backoff time of GCS JSON API requests #1207

arunkumarchacko · 2024-07-07T08:44:36Z

No description provided.

arunkumarchacko · 2024-07-07T08:44:45Z

/gcbrun

util/src/main/java/com/google/cloud/hadoop/util/GcsJsonApiEvent.java

singhravidutt · 2024-07-08T15:40:02Z

util/src/main/java/com/google/cloud/hadoop/util/GcsJsonApiEvent.java

+  }
+
+  @VisibleForTesting
+  GcsJsonApiEvent(@Nonnull HttpRequest request, EventType eventType) {


Why not just extract the meaningful information from HttpRequest and wrap it around RequestContext. Doing that will make it easier to extend it for gRPC as well.

For gRPC we can create another method or refactor this once we add it.

gRPC implementation is already there are we are even moving towards GA. Ideally, we should add this support for gRPC as well. Least we can do is, any new code added should be extendable to gRPC as well, else we are just be pilling on the tech debt.

gRPC support will come in a separate change.

singhravidutt · 2024-07-08T16:50:36Z

util/src/main/java/com/google/cloud/hadoop/util/GcsJsonApiEvent.java

+
+  private GcsJsonApiEvent(HttpRequest request, EventType eventType, int capacity) {
+    this(request, eventType);
+    this.properties = new HashMap<>(capacity);


What's the purpose of setting capacity? Looking into the usage of it in getBackoffEvent it seems it will immediately allocate extra capacity as well as rehash it.

This is done to limit the size of the map. BackOffEvent has two additional properties, hence setting the capacity to 2.

setting this capacity will not limit the size of hashMap. It's actually not capacity but initialCapacity
https://docs.oracle.com/javase/8/docs/api/java/util/HashMap.html#HashMap-int-

There is no way you will be able to limit the size of hashMap unless ofcourse you customize and override the implementation.

Am I missing something here?

If you provide the capacity, it will set the capacity to the next power of 2. It will not resize. What you think it will resize?

As per the documentation When the number of entries in the hash table exceeds the product of the load factor and the current capacity, the hash table is rehashed. With Default load factor of 0.75 it will rehash as soon as there are two entries in HashMap.

util/src/test/java/com/google/cloud/hadoop/util/RetryHttpInitializerTest.java

util/src/main/java/com/google/cloud/hadoop/util/RequestTracker.java

singhravidutt · 2024-07-10T11:40:27Z

util/src/test/java/com/google/cloud/hadoop/util/RetryHttpInitializerTest.java

+    }
+
+    @Override
+    protected RequestTracker getRequestTracker(HttpRequest request) {


Correct me If I am wrong, With this override what we are able to use the same RequestTracker instance against all the gcs requests made in test case i.e. getMetadata, getObject, listObject etc, all the retires of the request are also captured via same tracer but in actual production scenario it will not be the case, every request will have a separate tracker.

If yes, will it even be a valid test then?

yes, same tracker is used. Otherwise the test would have failed. Same things is happening in RetryHttpInitializer as well since since initialize is called only once.

Yeah, same tracker will be used across all retries of same request, but if there are multiple requests, say two different request to access object metadata of object1 and object2. TestRetryHttpInitializer configured here will not be able to provide different tracker but provide the same tracker which will yield weird outcomes at verification step.

will sync offline.

util/src/test/java/com/google/cloud/hadoop/util/RetryHttpInitializerTest.java

util/src/main/java/com/google/cloud/hadoop/util/RetryHttpInitializer.java

arunkumarchacko · 2024-07-10T14:23:53Z

/gcbrun

arunkumarchacko · 2024-07-11T04:28:16Z

/gcbrun

singhravidutt · 2024-07-11T04:03:04Z

util/src/main/java/com/google/cloud/hadoop/util/GcsJsonApiEvent.java

+
+  private GcsJsonApiEvent(HttpRequest request, EventType eventType, int capacity) {
+    this(request, eventType);
+    this.properties = new HashMap<>(capacity);


setting this capacity will not limit the size of hashMap. It's actually not capacity but initialCapacity
https://docs.oracle.com/javase/8/docs/api/java/util/HashMap.html#HashMap-int-

There is no way you will be able to limit the size of hashMap unless ofcourse you customize and override the implementation.

Am I missing something here?

singhravidutt · 2024-07-11T04:05:14Z

util/src/main/java/com/google/cloud/hadoop/util/GcsJsonApiEvent.java

+  }
+
+  @VisibleForTesting
+  GcsJsonApiEvent(@Nonnull HttpRequest request, EventType eventType) {


gRPC implementation is already there are we are even moving towards GA. Ideally, we should add this support for gRPC as well. Least we can do is, any new code added should be extendable to gRPC as well, else we are just be pilling on the tech debt.

singhravidutt · 2024-07-11T04:06:04Z

util/src/main/java/com/google/cloud/hadoop/util/RequestTracker.java

+import com.google.common.flogger.GoogleLogger;
+import java.util.concurrent.TimeUnit;
+
+class RequestTracker {


similar comment.

singhravidutt · 2024-07-11T04:09:32Z

util/src/main/java/com/google/cloud/hadoop/util/RequestTracker.java

+    postToEventQueue(GcsJsonApiEvent.getExceptionEveent(httpRequest));
+  }
+
+  void trackUnsuccessfulResponseHandler(HttpResponse response) {


trackResponse is called from response interceptor and trackUnsuccessfulResponseHandler is called from Unsuccessful response handler. I expect the responseInterceptor to be called no matter if response was unsuccessful. Weird.

singhravidutt · 2024-07-11T04:12:56Z

util/src/main/java/com/google/cloud/hadoop/util/RequestTracker.java

+      stopWatch.stop();
+
+      if (stopWatch.elapsed().toMillis() > LOGGING_THRESHOLD) {
+        logger.atInfo().atMostEvery(10, TimeUnit.SECONDS).log(


Yeah, but it's not capturing elapsedTime correct?

singhravidutt · 2024-07-11T06:01:38Z

util/src/test/java/com/google/cloud/hadoop/util/RetryHttpInitializerTest.java

+    }
+
+    @Override
+    protected RequestTracker getRequestTracker(HttpRequest request) {


Yeah, same tracker will be used across all retries of same request, but if there are multiple requests, say two different request to access object metadata of object1 and object2. TestRetryHttpInitializer configured here will not be able to provide different tracker but provide the same tracker which will yield weird outcomes at verification step.

util/src/test/java/com/google/cloud/hadoop/util/TestRequestTracker.java

singhravidutt · 2024-07-11T06:08:01Z

util/src/test/java/com/google/cloud/hadoop/util/TestRequestTracker.java

+          assertThat(backOffTime).isAtLeast(expectedBackoffTime);
+          assertThat(backOffTime).isLessThan(expectedBackoffTime + 10);


why it varies as range?
How we came up with 10?
will it cause flakiness?
what if some adds a test where many more retires are done, this verification will still be valid?

These are unit tests, not actual backoff is happening. The buffer is the potential overhead. I do not expect it to be more than 10 seconds.

These are unit tests, not actual backoff is happening.

Well, that is not always true, is it? One can mock the transport in unit tests but still use RetryHttpIniitalizer which comes with ExponentialBackOff strategy and hence the actual backoff will will happen.

util/src/test/java/com/google/cloud/hadoop/util/TestRequestTracker.java

util/src/main/java/com/google/cloud/hadoop/util/RequestTracker.java

arunkumarchacko · 2024-07-11T09:18:49Z

/gcbrun

arunkumarchacko · 2024-07-15T02:46:53Z

/gcbrun

arunkumarchacko · 2024-07-15T02:57:40Z

/gcbrun

arunkumarchacko · 2024-07-15T05:44:31Z

/gcbrun

arunkumarchacko · 2024-07-15T05:47:28Z

/gcbrun

…' into gcs-api-metrics-3-0

arunkumarchacko · 2024-07-15T06:25:47Z

/gcbrun

arunkumarchacko · 2024-07-15T09:52:10Z

/gcbrun

singhravidutt · 2024-07-15T12:26:57Z

util/src/test/java/com/google/cloud/hadoop/util/RetryHttpInitializerTest.java

@@ -166,4 +246,23 @@ private static RetryHttpInitializer createRetryHttpInitializer(Credentials crede
            .setReadTimeout(Duration.ofSeconds(5))
            .build());
  }
+
+  // Helper class which help provide a custom test implementation of RequestTracker


I would recommend adding a note on limitation of TestRetryHttpInitializer, which is, "it can only used against test with only one request because RequestTracker is initialized along with HttpInitializer".

This is an inner class and is an implementation detail. I think it is obvious from the code that this applies for one request per TestRetryHttpInitializer.

…eCloudDataproc#1207) * Track latency, retry and backoff time of GCS JSON API requests

#1225)

Track latency, retry and backoff time of GCS JSON API requests

e6ca057

arunkumarchacko requested review from guljain and singhravidutt July 8, 2024 03:43

singhravidutt reviewed Jul 10, 2024

View reviewed changes

Take CR comments

fc68ce1

Fix a formatting issue

0ce7aab

singhravidutt reviewed Jul 11, 2024

View reviewed changes

Take CR comments

9070b6a

Add elapsed time

5322054

Adding a comment

ffa2957

Merge branch 'branch-3.0.x' into gcs-api-metrics-3-0

d9c81e1

arunkumarchacko added 2 commits July 15, 2024 11:48

Take CR comments

8d98e71

Merge remote-tracking branch 'refs/remotes/origin/gcs-api-metrics-3-0…

137fc17

…' into gcs-api-metrics-3-0

singhravidutt reviewed Jul 15, 2024

View reviewed changes

singhravidutt approved these changes Jul 15, 2024

View reviewed changes

arunkumarchacko merged commit 1ba5bc2 into GoogleCloudDataproc:branch-3.0.x Jul 15, 2024
4 checks passed

arunkumarchacko deleted the gcs-api-metrics-3-0 branch July 15, 2024 12:34

arunkumarchacko added a commit to arunkumarchacko/hadoop-connectors that referenced this pull request Jul 15, 2024

Track latency, retry and backoff time of GCS JSON API requests (Googl…

b8ff8c9

…eCloudDataproc#1207) * Track latency, retry and backoff time of GCS JSON API requests

arunkumarchacko added a commit that referenced this pull request Jul 17, 2024

Track latency, retry and backoff time of GCS JSON API requests (#1207) (

d23a77d

#1225)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Track latency, retry and backoff time of GCS JSON API requests #1207

Track latency, retry and backoff time of GCS JSON API requests #1207

arunkumarchacko commented Jul 7, 2024

arunkumarchacko commented Jul 7, 2024

singhravidutt Jul 8, 2024

arunkumarchacko Jul 10, 2024

singhravidutt Jul 11, 2024

arunkumarchacko Jul 11, 2024

singhravidutt Jul 8, 2024

arunkumarchacko Jul 10, 2024

singhravidutt Jul 11, 2024

arunkumarchacko Jul 11, 2024

singhravidutt Jul 15, 2024

singhravidutt Jul 10, 2024

arunkumarchacko Jul 10, 2024

singhravidutt Jul 11, 2024

arunkumarchacko Jul 11, 2024

arunkumarchacko commented Jul 10, 2024

arunkumarchacko commented Jul 11, 2024

singhravidutt Jul 11, 2024

singhravidutt Jul 11, 2024

singhravidutt Jul 11, 2024

singhravidutt Jul 11, 2024

singhravidutt Jul 11, 2024

singhravidutt Jul 11, 2024

singhravidutt Jul 11, 2024

arunkumarchacko Jul 11, 2024

singhravidutt Jul 15, 2024

arunkumarchacko commented Jul 11, 2024

arunkumarchacko commented Jul 15, 2024

arunkumarchacko commented Jul 15, 2024

arunkumarchacko commented Jul 15, 2024

arunkumarchacko commented Jul 15, 2024

arunkumarchacko commented Jul 15, 2024

arunkumarchacko commented Jul 15, 2024

singhravidutt Jul 15, 2024

arunkumarchacko Jul 15, 2024

		assertThat(backOffTime).isAtLeast(expectedBackoffTime);
		assertThat(backOffTime).isLessThan(expectedBackoffTime + 10);

Track latency, retry and backoff time of GCS JSON API requests #1207

Track latency, retry and backoff time of GCS JSON API requests #1207

Conversation

arunkumarchacko commented Jul 7, 2024

arunkumarchacko commented Jul 7, 2024

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arunkumarchacko commented Jul 10, 2024

arunkumarchacko commented Jul 11, 2024

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 11, 2024

arunkumarchacko commented Jul 15, 2024

arunkumarchacko commented Jul 15, 2024

arunkumarchacko commented Jul 15, 2024

arunkumarchacko commented Jul 15, 2024

arunkumarchacko commented Jul 15, 2024

arunkumarchacko commented Jul 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment