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

Remove the old bolt implementation #12974

Closed

Conversation

zhulongcheng
Copy link
Contributor

@zhulongcheng zhulongcheng commented Mar 28, 2019

Closes #13030

Steps to remove the old bolt implementation:

  • Step-1: add KVStore.Describe, KVStore.Collect and kv.Service.ID methods
    // register and collect prometheus metrics
  • Step-2: replace bolt.Client with bolt.KVStore
  • Step-3: remove bolt.Client
    // after Step-2, the bolt.Client is unused, so it can be removed

Special notes:
The example_test.go as a duplicate of testing/keyvalue_log.go, so was removed.


Briefly describe your proposed changes:

  • CHANGELOG.md updated with a link to the PR (not the Issue)
  • Rebased/mergeable
  • Tests pass
  • http/swagger.yml updated (if modified Go structs or API)
  • Sign CLA (if not already signed)

@zhulongcheng zhulongcheng force-pushed the rm-bolt-inmem-impl branch 8 times, most recently from 3d54011 to f6cc0e8 Compare March 31, 2019 11:40
@zhulongcheng zhulongcheng marked this pull request as ready for review March 31, 2019 15:33
@zhulongcheng zhulongcheng changed the title WIP: remove the old bolt and inmem implementation Remove the old bolt implementation Apr 1, 2019
@kelwang kelwang requested a review from desa April 1, 2019 18:14
@kelwang
Copy link
Contributor

kelwang commented Apr 1, 2019

Looks good to me. @desa could you take a look at the metrics part?

Copy link
Contributor

@desa desa left a comment

Choose a reason for hiding this comment

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

Just one small comment. Other than that, it LGTM. Might be good to have @goller sign off though since he's most familiar with the phone-home metrics stuff.

bolt/kv.go Outdated Show resolved Hide resolved
"github.com/influxdata/influxdb/kit/prom/promtest"
)

func TestInitialKVStoreMetrics(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Hey @zhulongcheng , would you try to port the existing metrics_test.go code here?

I'd like to make sure the metrics are coming out correctly

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

@zhulongcheng zhulongcheng force-pushed the rm-bolt-inmem-impl branch 3 times, most recently from 4a3d810 to 58343dc Compare May 14, 2019 12:06
@zhulongcheng zhulongcheng force-pushed the rm-bolt-inmem-impl branch 4 times, most recently from f9277ec to 0807706 Compare May 14, 2019 13:52
@stale
Copy link

stale bot commented Aug 12, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Aug 12, 2019
@stale
Copy link

stale bot commented Aug 19, 2019

This issue has been automatically closed because it has not had recent activity. Please reopen if this issue is still important to you. Thank you for your contributions.

@stale stale bot closed this Aug 19, 2019
@zhulongcheng
Copy link
Contributor Author

Hey @kelwang @goller @desa , I'm splitting this PR into some small PRs.
(Likes #14726 #14727 #14503 ...)

New PRs will be review-friendly and low-risk.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove the old bolt implementation
4 participants