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

Truncate the shard group end time if it exceeds MaxNanoTime #6693

Merged
merged 1 commit into from
May 26, 2016

Conversation

jsternberg
Copy link
Contributor

@jsternberg jsternberg commented May 20, 2016

Related #6599.

@mention-bot
Copy link

By analyzing the blame information on this pull request, we identified @e-dard to be a potential reviewer

@jsternberg jsternberg force-pushed the js-6599-fix-shard-groups-near-max-time branch 2 times, most recently from ab1b1d0 to be4cd2d Compare May 20, 2016 14:48
} else if pb.EndTime != nil {
sgi.EndTime = UnmarshalTime(pb.GetEndTime())
} else {
return errors.New("unable to unmarshal EndTime, missing both EndTime and Duration")
Copy link
Contributor

Choose a reason for hiding this comment

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

s/missing both EndTime and Duration/missing either EndTime or Duration

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should this be a panic instead? It should never happen, but I feel like I have to account for this situation since both of the items are optional.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also it's missing both of them since this error only happens when pb.Duration == nil && pb.EndTime == nil.

@jsternberg jsternberg force-pushed the js-6599-fix-shard-groups-near-max-time branch from be4cd2d to 3ca2741 Compare May 20, 2016 15:06
@jsternberg
Copy link
Contributor Author

I updated the PR to serialize both the end time and the duration, but to always prefer the duration if it's there. This keeps it compatible if someone upgrades and wants to go back a version. Should I add the required keyword back to end time so we don't have to have the error checking?

@jsternberg jsternberg force-pushed the js-6599-fix-shard-groups-near-max-time branch from 3ca2741 to 3384f7c Compare May 20, 2016 15:42
@@ -371,6 +372,9 @@ func (data *Data) CreateShardGroup(database, policy string, timestamp time.Time)
sgi.ID = data.MaxShardGroupID
sgi.StartTime = timestamp.Truncate(rpi.ShardGroupDuration).UTC()
sgi.EndTime = sgi.StartTime.Add(rpi.ShardGroupDuration).UTC()
if sgi.EndTime.After(models.MaxNanoTime) {
sgi.EndTime = models.MaxNanoTime
Copy link
Contributor

Choose a reason for hiding this comment

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

In here also need to do sgi.StartTime = sgi.EndTime.Add(-models.MaxNanoTime.UnixNano()) or similar.

@jsternberg jsternberg changed the title Fix protobuf serialization of ShardGroupInfo near end time boundary Truncate the shard group end time if it exceeds MaxNanoTime May 20, 2016
@jsternberg jsternberg force-pushed the js-6599-fix-shard-groups-near-max-time branch 2 times, most recently from b712781 to 83dd10c Compare May 23, 2016 13:19
@jsternberg jsternberg added this to the 1.0.0 milestone May 25, 2016
@jsternberg jsternberg force-pushed the js-6599-fix-shard-groups-near-max-time branch from 83dd10c to 0d386e1 Compare May 25, 2016 16:18
@e-dard
Copy link
Contributor

e-dard commented May 25, 2016

@jsternberg this LGTM now 👍

@jsternberg jsternberg force-pushed the js-6599-fix-shard-groups-near-max-time branch from 0d386e1 to 907c88d Compare May 26, 2016 01:25
@jsternberg jsternberg merged commit 1d467ab into master May 26, 2016
@jsternberg jsternberg deleted the js-6599-fix-shard-groups-near-max-time branch May 26, 2016 02:04
@timhallinflux timhallinflux modified the milestones: 1.0.0, 1.0.0 beta Dec 20, 2016
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