Skip to content
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

Jump to bottom

adding support for invocationId in Json path #850

Merged
merged 6 commits into from
Aug 18, 2022
Merged

adding support for invocationId in Json path #850

merged 6 commits into from
Aug 18, 2022

Conversation

singhravidutt
Copy link
Contributor

No description provided.

@singhravidutt
Copy link
Contributor Author

/gcbrun

@codecov
Copy link

codecov bot commented Aug 11, 2022 •
edited
Loading

Codecov Report

Merging #850 (043e62c) into master (db12a53) will increase coverage by 0.00%.
The diff coverage is 80.95%.

@@            Coverage Diff            @@
##             master     #850   +/-   ##
=========================================
  Coverage     80.49%   80.49%           
- Complexity     1999     2007    +8     
=========================================
  Files           136      137    +1     
  Lines          8968     8989   +21     
  Branches       1031     1034    +3     
=========================================
+ Hits           7219     7236   +17     
- Misses         1334     1335    +1     
- Partials        415      418    +3
Flag Coverage Δ
integrationtest 62.68% <80.95%> (-0.05%) ⬇️
unittest 74.72% <80.95%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...oop/util/interceptors/InvocationIdInterceptor.java 80.00% <80.00%> (ø)
...google/cloud/hadoop/util/RetryHttpInitializer.java 85.52% <100.00%> (+0.19%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@singhravidutt singhravidutt marked this pull request as ready for review August 11, 2022 18:27
arunkumarchacko
arunkumarchacko reviewed Aug 16, 2022
View reviewed changes
gcsio/src/test/java/com/google/cloud/hadoop/gcsio/TrackingHttpRequestInitializer.java Show resolved Hide resolved
util/src/main/java/com/google/cloud/hadoop/util/interceptors/InvocationIdInterceptor.java Show resolved Hide resolved
util/src/main/java/com/google/cloud/hadoop/util/interceptors/InvocationIdInterceptor.java Outdated Show resolved Hide resolved
util/src/main/java/com/google/cloud/hadoop/util/interceptors/InvocationIdInterceptor.java Show resolved Hide resolved
util/src/main/java/com/google/cloud/hadoop/util/interceptors/InvocationIdInterceptor.java Show resolved Hide resolved
util/src/main/java/com/google/cloud/hadoop/util/interceptors/InvocationIdInterceptor.java Outdated Show resolved Hide resolved
util/src/main/java/com/google/cloud/hadoop/util/interceptors/InvocationIdInterceptor.java Outdated Show resolved Hide resolved
util/src/main/java/com/google/cloud/hadoop/util/interceptors/InvocationIdInterceptor.java Outdated Show resolved Hide resolved
util/src/main/java/com/google/cloud/hadoop/util/interceptors/InvocationIdInterceptor.java Show resolved Hide resolved
gcsio/src/test/java/com/google/cloud/hadoop/gcsio/TrackingHttpRequestInitializer.java Show resolved Hide resolved
@singhravidutt
Copy link
Contributor Author

/gcbrun

@singhravidutt
Copy link
Contributor Author

/gcbrun

arunkumarchacko
arunkumarchacko reviewed Aug 18, 2022
View reviewed changes
gcsio/src/test/java/com/google/cloud/hadoop/gcsio/TrackingHttpRequestInitializer.java Show resolved Hide resolved
gcsio/src/test/java/com/google/cloud/hadoop/gcsio/TrackingHttpRequestInitializer.java Show resolved Hide resolved
gcsio/src/test/java/com/google/cloud/hadoop/gcsio/TrackingHttpRequestInitializer.java Outdated Show resolved Hide resolved
gcsio/src/test/java/com/google/cloud/hadoop/gcsio/TrackingHttpRequestInitializer.java Show resolved Hide resolved
util/src/main/java/com/google/cloud/hadoop/util/interceptors/InvocationIdInterceptor.java Show resolved Hide resolved
util/src/main/java/com/google/cloud/hadoop/util/RetryHttpInitializer.java
@@ -95,6 +96,7 @@ public void initialize(HttpRequest request) throws IOException {
headers.setUserAgent(options.getDefaultUserAgent());
}
headers.putAll(options.getHttpHeaders());
request.setInterceptor(new InvocationIdInterceptor(request.getInterceptor()));
Copy link
Contributor

Choose a reason for hiding this comment

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

It is not obvious why we are passing one interceptor to another. Is it because this will override the existing interceptor? If yes, please add a comment since it is not very obvious. Also we use ChainingInterceptors at other places for similar scenarios.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nothing is getting overridden. All interceptors are getting chained via this. Unlike gRPC in HTTP there is no way to provide more than one interceptor for request and this feels like the only way via which we can have multiple interceptors(one after another) act on a single request.

ChainingInterceptors which you are referring to is not a interceptor but an initializer:ChainingHttpRequestInitializer which chains multiple initializers and interceptors using the same exact logic which I used (or other way round).

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, I was referring about ChainingInterceptors. Please do add a comment explaining that this is doing a chaining interceptor. Also why not make use of existing chaining interceptor?

Copy link
Contributor Author

@singhravidutt singhravidutt Aug 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

There is NO ChainingInterceptors there is [ChainingHttpRequestInitializer](https://github.com/GoogleCloudDataproc/hadoop-connectors/blob/master/util/src/main/java/com/google/cloud/hadoop/util/ChainingHttpRequestInitializer.java). I am not creating another HTTPInitializer just an interceptor and hence there is no way(or no need to) I can use ChainingHttpRequestInitializer. Interceptor and Initializer are two different things.
IIUC, If someone is creating a new interceptor and they can add it to any of the HTTPInitializer used.

util/src/main/java/com/google/cloud/hadoop/util/interceptors/InvocationIdInterceptor.java Show resolved Hide resolved
util/src/main/java/com/google/cloud/hadoop/util/interceptors/InvocationIdInterceptor.java Show resolved Hide resolved
util/src/main/java/com/google/cloud/hadoop/util/interceptors/InvocationIdInterceptor.java Outdated Show resolved Hide resolved
util/src/main/java/com/google/cloud/hadoop/util/interceptors/InvocationIdInterceptor.java Show resolved Hide resolved
@singhravidutt
Copy link
Contributor Author

/gcbrun

@singhravidutt
Copy link
Contributor Author

/gcbrun

@singhravidutt
Copy link
Contributor Author

/gcbrun

@singhravidutt singhravidutt merged commit de53e52 into master Aug 18, 2022
singhravidutt added a commit that referenced this pull request Sep 12, 2022
@singhravidutt
adding support for invocationId in Json path (#850)
9310831 
* adding support for invocationId in Json path

* addressed review comments

* addressed review comments

* addressed review comments

* addressed review comments
singhravidutt added a commit that referenced this pull request Sep 12, 2022
@singhravidutt
adding support for invocationId in Json path (#850) (#873)
bea3108 
* adding support for invocationId in Json path (#850)

* adding support for invocationId in Json path

* addressed review comments

* addressed review comments

* addressed review comments

* addressed review comments

* formatting changes

* fixed Unit test

* formatting changes

* Fixing integration test

* formatting changes

* fixed integration test
@singhravidutt singhravidutt deleted the json_invocationId branch November 15, 2022 09:42
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arunkumarchacko arunkumarchacko arunkumarchacko approved these changes

@Deependra-Patel Deependra-Patel Awaiting requested review from Deependra-Patel

@medb medb Awaiting requested review from medb

@abmodi abmodi Awaiting requested review from abmodi

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@singhravidutt @arunkumarchacko