Skip to content

Conversation

aaron-steinfeld
Copy link
Contributor

Description

This channel registry, ported from https://github.com/hypertrace/hypertrace-core-graphql/blob/7df21a96bf0712e239ced1117c49889870cf7b3e/hypertrace-core-graphql-grpc-utils/src/main/java/org/hypertrace/core/graphql/utils/grpc/DefaultGrpcChannelRegistry.java is used by clients to prevent multiple places in code from instantiating the same channel (which grpc strongly discourages), instead sharing channels and allowing centralized shutdown.

Testing

Added UT

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

public class GrpcChannelRegistry {
private static final Logger LOG = LoggerFactory.getLogger(GrpcChannelRegistry.class);
private final Map<String, ManagedChannel> channelMap = new ConcurrentHashMap<>();
private volatile boolean isShutdown = false;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed the integrated shutdown on this one so it didn't bring in more dependencies (the shutdown lifecycle is from), but I can bring it back if that seems more useful. The more I think on it, the more I think I'll optionally add just the future part of it (to avoid the dependency)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed my mind again - the difference of accepting a hook is really just the below anyway:

new GrpcChannelRegistry(future);
// vs
registry = new GrpcChannelRegistry();
future.thenRun(registry::shutdown);

@github-actions

This comment has been minimized.

@codecov
Copy link

codecov bot commented Mar 25, 2021

Codecov Report

Merging #13 (1c7a2a6) into main (b52c268) will increase coverage by 1.50%.
The diff coverage is 84.61%.

Impacted file tree graph

@@             Coverage Diff              @@
##               main      #13      +/-   ##
============================================
+ Coverage     64.96%   66.47%   +1.50%     
- Complexity       51       58       +7     
============================================
  Files            13       14       +1     
  Lines           157      170      +13     
  Branches          9        9              
============================================
+ Hits            102      113      +11     
  Misses           49       49              
- Partials          6        8       +2     
Flag Coverage Δ Complexity Δ
unit 66.47% <84.61%> (+1.50%) 0.00 <7.00> (ø)

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

Impacted Files Coverage Δ Complexity Δ
...ace/core/grpcutils/client/GrpcChannelRegistry.java 84.61% <84.61%> (ø) 7.00 <7.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 b52c268...1c7a2a6. Read the comment docs.

@aaron-steinfeld aaron-steinfeld merged commit e1a88d1 into main Mar 25, 2021
@aaron-steinfeld aaron-steinfeld deleted the add-channel-registry branch March 25, 2021 13:23
@github-actions
Copy link

Unit Test Results

  9 files  +1    9 suites  +1   7s ⏱️ ±0s
42 tests +4  42 ✔️ +4  0 💤 ±0  0 ❌ ±0 

Results for commit e1a88d1. ± Comparison against base commit b52c268.

@aaron-steinfeld aaron-steinfeld added the enhancement New feature or request label Mar 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants