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

Simple StoreFinder and new capacity gossiping #187

Merged
merged 5 commits into from
Nov 26, 2014
Merged

Simple StoreFinder and new capacity gossiping #187

merged 5 commits into from
Nov 26, 2014

Conversation

embark
Copy link
Contributor

@embark embark commented Nov 25, 2014

As discussed, capacity gossip is now gossiped per store, not by group, at least for the beta release.

Store now tracks gossip keys of store capacity, and scans the capacity gossip
for attribute matches when they need to make allocation decisions. A simple,
linear implementation that has a straightforward way to implement garbage
collection of capacity keys, but faster methods can be employed later. It
is might be sufficient as long as it is sufficient for all stores to gossip
capacity, but will need to be reevaluated when capacity is gossiped through
groups

This should be sufficient for the beta stages. When more stores
are added, then switch to gossiping capacity through groups,
so that only the top N will be gossiped. This will lower the gossip
and storage load on all machines.
Store now tracks gossip keys of store capacity, and scans the capacity gossip
for attribute matches when they need to make allocation decisions. A simple,
linear implementation that has a straightforward way to implement garbage
collection of capacity keys, but faster methods can be employed later. It
is likely sufficient as long as it is sufficient for all stores to gossip
capacity, but will need to be reevaluated when capacity is gossiped through
groups.
@@ -0,0 +1,88 @@
// Copyright 2014 The Cockroach Authors.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

All of this could be done in storage/store.go, I just felt that file was getting pretty long. Willing to change back at request!

Copy link
Member

Choose a reason for hiding this comment

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

Why not combine the code in this file which adds methods to the Store struct into a StoreFinder struct and then embed that in Store? I dislike having methods defined in separate files for the same struct.

Currently "StoreFinder" is a func type. We could consider just renaming that FindStoreFunc

Copy link
Member

Choose a reason for hiding this comment

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

Or don't embed it...just keep it as a constituent object. That's fine too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you suggesting making the below methods defined on the finderContext or new StoreFinder struct instead of methods defined directly on Store?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, my instinct would be to create a StoreFinder struct and add these methods to it. Then just create an instance of StoreFinder when creating a new Store. Embedding might make sense too, but I don't think it matters in this context.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, I tried the embedding method. Looks reasonable -- PTAL

@spencerkimball
Copy link
Member

LGTM

Changes integer base on capacity gossip keys and adds TODO
for possible speedup.
In response to Spencer's PR comments, rather than increasing
the length of storage/store by defining store finder methods on Store itself.
@spencerkimball
Copy link
Member

LGTM

embark pushed a commit that referenced this pull request Nov 26, 2014
Simple StoreFinder and new capacity gossiping
@embark embark merged commit b856d2c into cockroachdb:master Nov 26, 2014
@embark embark deleted the embark/rebalancing branch November 26, 2014 20:28
koorosh pushed a commit to koorosh/cockroach that referenced this pull request May 13, 2021
cluster-ui: sessions redux functionality
pav-kv pushed a commit to pav-kv/cockroach that referenced this pull request Apr 10, 2024
…h-assert-quorum

Test: Replace t.error/fatal with assert/request in [/quorum]
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.

2 participants