Comparisons between TserverInstance and TabletMetadata.Location are error prone

[keith-turner]

While working on #3251 an equals method was added to TabletMetadata.Location which caused this test to break.

The cause of the breakage in the test was this code that ends up looking up a TabletMetadata.Location object in a set of TServerInstance objects. This works because TabletMetadata.Location extends TServerInstance and does not override equals or hashcode. Overriding the equals method caused the test to break.

It would be much cleaner and less error prone if TabletMetadata.Location encapsulated TServerInstance instead of extending it. If something wanted to compare to the TServerInstance then it could call getTServerInstance() on location and compare to that. Encapsulation instead of extension would allow TabletMetadata.Location to have its own sensible equals and hashcode methods also.

[ctubbsii]

This is a similar problem to the TabletFile/StoredTabletFile issue that @EdColeman and I were previously looking into (the "as stored" format being analogous to "location" here as the type to be encapsulated). However, I think this one might be a bit easier to fix.

[cshannon]

I also was thinking the same thing that this looked similar to #2830

I can can ahead and fix it.

[cshannon]

Should this be targeted for 2.1.1 or 3.0?

I ask because it is a bug fix but it would be nice to be able to change the API, I noticed in Ample we are passing both the tserver and the location type as arguments but we could just pass the Location as that contains both but would require changing the interface.

accumulo/core/src/main/java/org/apache/accumulo/core/metadata/schema/Ample.java

Lines 260 to 262 in 7d69842

	TabletMutator putLocation(TServerInstance tserver, LocationType type);
	TabletMutator deleteLocation(TServerInstance tserver, LocationType type);

It would be nicer to have:

TabletMutator putLocation(TabletMetadata.Location location);

TabletMutator deleteLocation(TabletMetadata.Location location);

[cshannon]

I think I might try and keep the scope of this smaller for now as doing a bunch of refactoring has a lot of changes and would be nice to fix the issue with a smaller change and refactor later if we wanted.

[keith-turner]

Should this be targeted for 2.1.1 or 3.0?

Its not public API so we can change it in 2.1.1 if we want. In its current state it is tricky code and anyone writing new code against it would have to be really careful to understand its nuances (like calling equals on two location objects will not behave in an expected way). In my opinion its worth attempting to fix in 2.1, but I don't know how extensive the fix will end up being. Personally I would start in 2.1 and if it started to feel like the fix might cause new bugs then I might only fix it in 3.

Yesterday I took a quick look at how many things called TabletMetadata.getLocation and it was around 50, but didn't further investigate how many of those might be doing equal, set, or map operations with the objects. I don't have a good feel for how much code the fix will end up touching.

[cshannon]

Should this be targeted for 2.1.1 or 3.0?

Its not public API so we can change it in 2.1.1 if we want. In its current state it is tricky code and anyone writing new code against it would have to be really careful to understand its nuances (like calling equals on two location objects will not behave in an expected way). In my opinion its worth attempting to fix in 2.1, but I don't know how extensive the fix will end up being. Personally I would start in 2.1 and if it started to feel like the fix might cause new bugs then I might only fix it in 3.

Yesterday I took a quick look at how many things called TabletMetadata.getLocation and it was around 50, but didn't further investigate how many of those might be doing equal, set, or map operations with the objects. I don't have a good feel for how much code the fix will end up touching.

Alright, I just had commented about doing a smaller change but if we are ok with API changes since it's not public it's probably better to just refactor it all to fix it up in one shot so I will work on that.

[keith-turner]

It would be nicer to have:

TabletMutator putLocation(TabletMetadata.Location location);

TabletMutator deleteLocation(TabletMetadata.Location location);

That does seem nicer. That could be another PR/issue or done in the same as this if its aligned with the changes.