Don't precreate shard groups entirely in past

[otoolep]

If a shard group is completely in the past, don't precreate its successor. Before this fix, this could happen if someone wrote a single point into the past, which means a shard group was created for that point, and then precreation would create the entire successor series of groups.

Fixes issue #3722

[otoolep]

@corylanou

[corylanou]

Nice! Good optimization. +1 on green.

[pauldix]

The PrecreateShardGroups function looks more complex to me than it should be. The ShardGroups in any retention policy should always be sorted in time ascending order. That means you should only look at the last shard group for each RP.

Also, cutoff time doesn't make sense to me. I think it's easier to understand if it something like this inside the loop of the replication policies.

mostRecentShard := rp.ShardGroups[len(rp.ShardGroups)-1]
nearFuture := time.Now.Add(2 * time.Hour).Unix()
if nearFuture > mostRecentShard.EndTime.Unix() {
  // create the shard group
}

[otoolep]

@pauldix -- cutoff time is nearFuture in your version, the logic is the same. We might make it easier to follow, but the core logic remains the same. Also, the choice to push the various times into the function is about making testing easier and cleaner.

Let me see if I can simplify it by looking at only the last shard group in the RP.

[otoolep]

Shard groups are not stored in a sorted manner, that I can see.

https://github.com/influxdb/influxdb/blob/master/meta/data.go#L353

and the function which returns ShardGroups on an RPI, doesn't seem to sort them either:

https://github.com/influxdb/influxdb/blob/master/meta/data.go#L241

We might need some more changes here, such that the Precreate function can be simplified. I can do that in a separate PR.

[pauldix]

Yeah, since shard groups are rarely created, and they almost always need to
be queried in sorted order, they should be kept sorted. That way we don't
need to sort them every time before querying.

On Tue, Sep 1, 2015 at 3:37 PM, otoolep notifications@github.com wrote:

Shard groups are not stored in a sorted manner, that I can see.

https://github.com/influxdb/influxdb/blob/master/meta/data.go#L353

and the function which returns ShardGroups on an RPI, doesn't seem to sort
them either:

https://github.com/influxdb/influxdb/blob/master/meta/data.go#L241

We might need some more changes here, such that the Precreate function can
be simplified. I can do that in a separate PR.

—
Reply to this email directly or view it on GitHub
#3931 (comment).

[otoolep]

Agreed @pauldix -- the case for storing these sorted makes sense to me. Let me see what I can do.

[otoolep]

@pauldix @corylanou

Logic updated now that shard groups are sorted by time. Removes one for loop and if check, which is good. I also renamed cutoff to future. I am open to other names.

[jwilder]

A comment here would be useful. When would len(rp.ShardGroups) == 0? Is this basically checking to make sure that the retention policy has been written to at least once before pre-creating shard groups?

[otoolep]

Yes, exactly. Or all shard groups have been expired out.

[jwilder]

Two small smalls things but otherwise 👍

[otoolep]

Thanks @jwilder -- will merge on green.