Skip to content

Attribute Service Client Configurable Timeout#71

Merged
aaron-steinfeld merged 5 commits intohypertrace:mainfrom
suddendust:config_timeout
Jul 12, 2021
Merged

Attribute Service Client Configurable Timeout#71
aaron-steinfeld merged 5 commits intohypertrace:mainfrom
suddendust:config_timeout

Conversation

@suddendust
Copy link
Copy Markdown
Contributor

@suddendust suddendust commented Jul 11, 2021 •
edited
Loading

Description

Like the PR here, this change is to make client timeout for the Attribute Service configurable.

Testing

Deployed and tested the application locally. Scenarios:

  1. Missing config: Default timeout is used.
  2. Timeout configured: Configured value is used.

Checklist:

  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • Any dependent changes have been merged and published in downstream modules

@suddendust suddendust requested a review from a team as a code owner July 11, 2021 06:25
@codecov
Copy link
Copy Markdown

codecov Bot commented Jul 11, 2021 •
edited
Loading

Codecov Report

Merging #71 (bc65020) into main (2b889a0) will decrease coverage by 0.09%.
The diff coverage is 0.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##               main      #71      +/-   ##
============================================
- Coverage     59.30%   59.20%   -0.10%     
  Complexity      239      239              
============================================
  Files            79       79              
  Lines          1209     1211       +2     
  Branches         34       34              
============================================
  Hits            717      717              
- Misses          464      466       +2     
  Partials         28       28
Flag Coverage Δ
unit 59.20% <0.00%> (-0.10%) ⬇️

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

Impacted Files Coverage Δ
...trace/core/graphql/attributes/AttributeClient.java 0.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2b889a0...bc65020. Read the comment docs.

@suddendust suddendust mentioned this pull request Jul 11, 2021
Open
3 tasks
@suddendust
Copy link
Copy Markdown
Contributor Author

suddendust commented Jul 11, 2021

@aaron-steinfeld Same changes what we did for Gateway service. Please review once you're back. Thanks :)

kotharironak
kotharironak previously approved these changes Jul 12, 2021
View reviewed changes
@kotharironak
Copy link
Copy Markdown
Contributor

kotharironak commented Jul 12, 2021

@suddendust Can you take care of snyk failure? For no upgrade lib, can you update the .snyk files?

@kotharironak kotharironak self-requested a review July 12, 2021 07:31
kotharironak
kotharironak previously approved these changes Jul 12, 2021
View reviewed changes
JBAhire
JBAhire previously approved these changes Jul 12, 2021
View reviewed changes
JBAhire
JBAhire reviewed Jul 12, 2021
View reviewed changes
Comment thread ...ql-attribute-store/src/main/java/org/hypertrace/core/graphql/attributes/AttributeClient.java
AttributeModelTranslator translator) {
this.grpcContextBuilder = grpcContextBuilder;
this.translator = translator;
this.serviceConfig = serviceConfig;
Copy link
Copy Markdown
Member

@JBAhire JBAhire Jul 12, 2021

Choose a reason for hiding this comment

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

Can you please add unit test for this?

Copy link
Copy Markdown
Contributor Author

@suddendust suddendust Jul 12, 2021

Choose a reason for hiding this comment

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

Hi Jayesh, sure I can add a UT for this (was even thinking of doing it before raising the PR). However, @aaron-steinfeld had this to say about adding UTs to these. There isn't much I can test and assert on here (except that the test config is read correctly). If you still think a UT is required, I'll gladly add it :)

Copy link
Copy Markdown
Member

@JBAhire JBAhire Jul 12, 2021

Choose a reason for hiding this comment

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

Got it! agreed!

aaron-steinfeld
aaron-steinfeld previously approved these changes Jul 12, 2021
View reviewed changes
Comment thread ...l-service/src/main/java/org/hypertrace/core/graphql/service/DefaultGraphQlServiceConfig.java
@aaron-steinfeld aaron-steinfeld merged commit 189b44f into hypertrace:main Jul 12, 2021
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Jul 12, 2021

Unit Test Results

29 files  ±0  29 suites  ±0   23s ⏱️ -1s
94 tests ±0  94 ✔️ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit 189b44f. ± Comparison against base commit 2b889a0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@JBAhire JBAhire JBAhire left review comments

@aaron-steinfeld aaron-steinfeld aaron-steinfeld approved these changes

@kotharironak kotharironak kotharironak left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@suddendust @kotharironak @JBAhire @aaron-steinfeld