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.

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

Replace closer with resource package #2864

Merged
merged 20 commits into from
Nov 9, 2020

Conversation

rallen090
Copy link
Collaborator

What this PR does / why we need it:

x/close is a subset of x/resource and so we've replaced usage of the former with the latter and removed the former.

Special notes for your reviewer:

Does this PR introduce a user-facing and/or backwards incompatible change?:


Does this PR require updating code package or user-facing documentation?:


@@ -36,7 +36,7 @@ type OptionsManager interface {
// RegisterWatcher registers a watcher that watches updates to runtime options.
// When an update arrives, the manager will deliver the update to all registered
// watchers.
RegisterWatcher(l OptionsWatcher) close.SimpleCloser
RegisterWatcher(l OptionsWatcher) resource.Closer
Copy link
Collaborator

Choose a reason for hiding this comment

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

I know we're not doing anything about it right now, but we'll eventually want to either (a) promote resource to non-x/ or (b) create per-package interfaces instead of bleeding x/ types as part of non-x/ APIs.

src/dbnode/client/connection_pool.go Show resolved Hide resolved
@@ -360,17 +359,6 @@ func openFiles(opener fileOpener, fds map[string]**os.File) error {
return firstErr
}

// TODO(xichen): move closeAll to m3x/close.
Copy link
Collaborator

Choose a reason for hiding this comment

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

👏

src/x/resource/types.go Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Nov 9, 2020

Codecov Report

Merging #2864 (ce720e8) into master (ce720e8) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #2864   +/-   ##
======================================
  Coverage    69.4%   69.4%           
======================================
  Files        1099    1099           
  Lines       99879   99879           
======================================
  Hits        69399   69399           
  Misses      25794   25794           
  Partials     4686    4686           
Flag Coverage Δ
aggregator 75.9% <0.0%> (ø)
cluster 85.1% <0.0%> (ø)
collector 84.3% <0.0%> (ø)
dbnode 79.1% <0.0%> (ø)
m3em 74.4% <0.0%> (ø)
m3ninx 70.3% <0.0%> (ø)
metrics 17.2% <0.0%> (ø)
msg 74.2% <0.0%> (ø)
query 56.9% <0.0%> (ø)
x 80.3% <0.0%> (ø)

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


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 ce720e8...b075d34. Read the comment docs.

Copy link
Collaborator

@wesleyk wesleyk left a comment

Choose a reason for hiding this comment

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

nice! LGTM

@robskillington
Copy link
Collaborator

LGTM but I don't think you meant to upgrade .ci submodule did you?

@rallen090
Copy link
Collaborator Author

LGTM but I don't think you meant to upgrade .ci submodule did you?

Yeah that was by mistake. Fixed.

@rallen090 rallen090 merged commit c6a256d into master Nov 9, 2020
@rallen090 rallen090 deleted the ra/close-replaced-with-resource-package branch November 9, 2020 22:19
soundvibe added a commit that referenced this pull request Nov 11, 2020
* master: (28 commits)
  [dbnode] Add claims for index segments volume index (#2846)
  [dbnode] Remove namespaces from example config and integration tests (#2866)
  [dbnode] Resurrect flaky test skip (#2868)
  [aggregator] Fix checkCampaignStateLoop (#2867)
  [dbnode] implement deletion method in namespace kvadmin service (#2861)
  Replace closer with resource package (#2864)
  Add coding style guide (#2831)
  Add GOVERNANCE.md to describe governance (#2830)
  Add COMPATIBILITY.md to describe version compatibility (#2829)
  Refactor etcd config as discovery section with convenience types (#2843)
  Refactor x/lockfile into dbnode/server (#2862)
  [lint] Disable nlreturn linter (#2865)
  [m3cluster] Expose placement algorithm in placement service (#2858)
  [etcd] Set reasonable cluster connection/sync settings by default (#2860)
  [dbnode] Use bits.LeadingZeros64 to improve encoder performance (#2857)
  Cleanup m3nsch leftovers (#2856)
  Update ci-scripts to correct coverage tracking (#2854)
  [aggregator] Process campaign state without waiting for first campaign check interval (#2855)
  Bump go to 1.14 (#2853)
  [query] Remove single series error from M3
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants