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

Fix for merge values #6587

Merged
merged 3 commits into from
May 10, 2016
Merged

Fix for merge values #6587

merged 3 commits into from
May 10, 2016

Conversation

jwilder
Copy link
Contributor

@jwilder jwilder commented May 10, 2016

Required for all non-trivial PRs
  • Rebased/mergable
  • Tests pass

Discovered two cases where Merge would sometimes return values unsorted. This PR fixes those cases. Also discovered some heavy lock contention in CreateShardGroup which this PR also fixes.

@mention-bot
Copy link

By analyzing the blame information on this pull request, we identified @joelegasse and @e-dard to be potential reviewers

@jwilder jwilder added this to the 1.0.0 milestone May 10, 2016
@@ -694,11 +694,19 @@ func (c *Client) DropShard(id uint64) error {

// CreateShardGroup creates a shard group on a database and policy for a given timestamp.
func (c *Client) CreateShardGroup(database, policy string, timestamp time.Time) (*ShardGroupInfo, error) {
// Check under a read-lock
Copy link
Contributor

Choose a reason for hiding this comment

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

I think a little more detail explaining the reason for the optimistic check under read lock would be worthwhile.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

CreateShardGroup is actually CreateShardGroupIfNotExists and shard groups are checked for existence much more frequently than they are created. The write lock showed up in the block profile when writing points w/ many fields.

@e-dard
Copy link
Contributor

e-dard commented May 10, 2016

LGTM 👍 Just that question above over whether you could reduce allocations by waiting until you know if you need to merge in an element before reducing the length of b.

@jwilder jwilder merged commit d8490f1 into master May 10, 2016
@jwilder jwilder deleted the jw-validate-fields branch May 10, 2016 17:56
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.

3 participants