Add Mergeability column to support automatic merges

[cshannon]

Note: The following description as been updated now that SteadyTime is stored separately in the mergeability metadata

This adds a new Mergeability column for marking if/when tablets are eligible to me merged by the system based on a threshold. The column stores two values, a duration which is a delay for when a tablet can be merged that is relative to the time the Manager uses, Steady time. It also stores the current Steady time value when inserted so that later we can add the delay plus the original time when inserted to see if enough time has passed. The Steady Time value will only be stored if the delay is >= 0, otherwise it will be null as it won't be used. The Steady Time value isn't technically needed for a delay of 0 because 0 means it's eligible to merge always, but it could be useful if we wanted to change the value for some reason and it would allow logging when it was stored.

There are 2 possible states for the delay value:

1. null : This means a tablet will never automatically merge
2. duration >= 0 : Tablet is eligible to merge after the given delay (stored as a duration), relative
   to the current system Steady time. Ie. the tablet can be merged if the current manager time is later than the delay value + the steady time value when inserted. If the duration is 0 then it means it can merge always (now).

This change only adds the new column itself and populates it. The default is to never merge automatically for all cases except for when the system automatically splits tablets. In that case the newly split tablets are marked as being eligible to merge always (duration of 0).

Future updates will add API enhancements to allow setting/viewing the mergeability setting as well as to enable automatic merging by the system that is based on this new column value.

When automatic merging is enabled, if a user wants to make a tablet eligible to be merged in the future they would do so by setting a delay that is positive. For example, to make a tablet eligible to be merged 3 days from now the user set a duration of 3 days in the API (future PR) and when the system inserts the value into metadata it will also include the current SteadyTime on creation. Later when the the current steady time passes that set delay + original stored steady time value the tablet would be eligible to be merged. To enable merging immediately they can set the duration to 0 or use the TabletMergeability.always() helper which is just a shortcut to a duration of 0.

See #5014 for more details

[dlmarion]

We may want to convert this to ms or s in the toString method for ease of understanding.

[cshannon]

Yeah, not a bad idea, nanoseconds would be pretty tough to understand in a log.

[keith-turner]

Could convert to ms and include the unit at the end of the string.

[keith-turner]

Eventually we will need a steady time to evaluate if something should merge. Seems like the steady time should always be set when the mergability is set. Although its only needed when the duration is > 0. The steady time could be encoded in the same columns value.

Suggested change

	T putTabletMergeability(TabletMergeability tabletMergeability);
	T putTabletMergeability(TabletMergeability tabletMergeability, SteadyTime steadTime);

[keith-turner]

Steady time could be added in later PRs. Just wondering how it will be persisted.

[cshannon]

My original plan was to persist everything as one value by adding the current manager time to the value. So basically to compute the mergability duration you would read the current SteadyTime then add the offset to it and store it and then later you could compare to the current time by taking a diff.

However, I was thinking more about it and I think that storing it as two separate values makes sense because it allows you to do better debugging (logigng) plus you can see extra information such as when the value was created. It also allows doing other things like update/replacing the original SteadyTime, etc and other metric calculations if we keep it separate so I'll change it.

[keith-turner]

My original plan was to persist everything as one value by adding the current manager time to the value.

Ok I had thought this would store the exact duration specified by the user. Summing the steady time and duration from the user would work. There may be a slight advantage for keeping them separate for debugging purposes as mentioned.

[cshannon]

@keith-turner - The latest update adds a new class just to wrap the metadata and make it easier to validate and store the delay + the steady time as a separate value. The intent is that the the client will still only use and pass TabletMergeability because only the Manager will be touching TabletMergeabilityMetadata as it just exists in Ample. A user would just set the TabletMergeability and if a delay is set then the Manager will set the SteadyTime value when inserting TabletMergeabilityMetadata.

[keith-turner]

This looks good. I made some subjective/stylistic comments that could be ignored.

[keith-turner]

Need @since tags in javadoc since this class is in a public API package.

[keith-turner]

This method needs javadoc outlining the acceptable input values for duration. Since this is a public API class wondering if the following would be better instead, however those methods would still need javadoc. Could make those constants private if adding these also, thinking the methods give a bit more flexibility to change.

Suggested change

	public static TabletMergeability from(Duration delay) {
	Preconditions.checkArgument(delay.toNanos() >= -1,
	"Duration of delay must be -1, 0, or a positive offset.");
	return new TabletMergeability(delay);
	}
	public static TabletMergeability now(){
	return NOW:
	}
	public static TabletMergeability never(){
	return NEVER:
	}

[cshannon]

This is a good idea and I may do this as well in TabletMergeabilityMetadata. The reason is I have been thinking that the NOW, NEVER etc constants that I defined in TabletMergeability and TabletMergeabilityMetadata could lead to future mistakes if developers forget they are not enums and incorrectly use an identity check for equality. At first glance TabletMergeability.NOW looks like an enum value when it isn't. In theory if the code always uses the constant then an identity check for equality would be true when checked, but it would still be safer and more correct to use .equals() and not == since it isn't an enum and both TabletMergeability and TabletMergeabilityMetadata have equals/hashcode defined.

[cshannon]

Also I will add some javadocs and we can tweak them in future PRs if things evolve/change which should be fine as long as we do it before we release 4.0.0

[keith-turner]

Feel free to ignore this suggestion. Was thinking this name went better w/ the getDelay() method.

Suggested change

	public boolean isFuture() {
	public boolean isDelayed() {

[cshannon]

I think isDelayed() is probably better and will rename it.

I kept going back and forth with trying to decide on the correct names and I think this may change anyways. I've been wondering if instead of NOW it should be ALWAYS or IMMEDIATE etc as those names could align better with NEVER. I will probably leave the name alone for now because I think the exact naming is going to be how we intent to use this going forward and if we want to rename it and TabletMergeability itself could possibly be renamed in a future PR.

Right now the name is TabletMereability but as @ctubbsii had pointed out in a previous conversation, the current goal for this is really tablet automatic mergeability so we can use it to determine if we can run an automatic merge on a tablet range just like we do automatic splits.

However, there has been some talk of making it more generic to mergeabilty in general (Even regular user triggered merges) which may also impact how we name things but not sure we will or need to. Either way, I expect in future PRs as this evolves the naming is going to be tweaked a bit.

[cshannon]

@keith-turner - The more I think about it, the more I'm not sure I like using NOW because it's a bit misleading. A tablet could be eligible to be merged now even if a delay is set if enough time has passed. So the value isn't really signaling that it's eligible now so much as that it's always eligible no matter what because the delay is 0 so I'm back to thinking maybe we rename it to something like ALWAYS or IMMEDIATELY.

[keith-turner]

Oh yeah NOW is kinda tricky. I like ALWAYS. Maybe could have two states instead of three by making delay be >=0 instead of >0. Then its just NEVER or DELAYED. NEVER is the one that is nice to specifically designate intention. From an encoding standpoint that would be a boolean and a long.

[cshannon]

This is an interesting idea, I think that makes sense to allow a delay of >= 0 because it fits in nicely as part of the regular delay logic but NEVER is the edge case as you said. For the json encoding we keep the boolean and long but inside the TabletMergeability class maybe we can just represent "NEVER" with a null delay. I think we should still keep the always() method shortcut I think because it's going to be a commonly used value.

[keith-turner]

Could do the following to give more flexibility w/ changing how now and never are represented, would need add javadoc if doing so.

Suggested change

	public Duration getDelay() {
	return delay;
	public Duration getDelay() {
	Preconditions.checkstate(isDelayed());
	return delay;

[cshannon]

I didn't end up making this change yet because it would make things a bit tricky for things like json serialization. It would make the serialization harder because the json code would now have to duplicate business logic and know to translate NEVER to -1 and NOW to 0 for the delay. This is the spot I am talking about.

That code would have to make sure to not call getDelay() unless it was positive and we would introduce multiple locations in the code that need to be updated if we wanted to change the meaning of -1 and 0. They are different packages so we can't make it package scope for the private delay either.

All that being said, I think this is doable and not a bad idea but if we do this I think it probably makes sense to completely decouple the serialization as well if the intent is to be more flexible in the future and no other code should really know what -1 and 0 mean outside of the internals of TabletMergeability. This would mean only allowing publicly accessible methods that create a TabletMergeability to set a delay of > 0 and requiring the use of TabletMergeability.now() or TabletMergeability.never() for all other cases so this method would go away.

For serialization, I think the json format in TabletMergeabilityMetadata would need to be changed to something like the following:

private static class GSonData {
  boolean never;
  boolean now;
  Long delay;
  Long steadyTime;
}

and we would just have to set steadyTime and delay to null values if either never or now was true.

@keith-turner - thoughts?

[cshannon]

Taking this one step further you could even just require the delay in TabletMergeability to always be positive and never < 0 and add a couple boolean flags to signal NOW and NEVER and just make the delay null in that case vs using 0 or -1. It probably doesn't matter either way though if we keep using 0 and -1 as it would be hidden internally to the class and not exposed.

[keith-turner]

Now its documented on the javadoc what -1,0, and >0 mean, which seems good for now. Its nice to structure the API so that the -1 case does not cause bugs in code calling getDelay AND to make it more flexible to change, but not really sure of the best way to achieve those goals. The javadoc lets someone know hey you need to handle this -1 case, which helps w/ the usage bug case. Making the API more flexible is tricky and as you mentioned this will probably evolve as more of the feature is implemented also, so it would be good to see what is learned from that.

If the code is released w/ that javadoc then its behavior is set and it would be ok for the serialization code to be tightly coupled to the behavior in the javadoc.

[keith-turner]

Could convert to ms and include the unit at the end of the string.

[cshannon]

I'm going to go ahead and merge this because there will be follow on PRs anyways so those can address changes going forward (which I'm sure there will be) as this feature evolves with the client API and the Fate code for handling automatic merging. I will be working on the client API changes next for setting the mergeability value of a table and tablets.