You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by "keith-turner (via GitHub)" <gi...@apache.org> on 2023/03/23 22:25:11 UTC
[GitHub] [accumulo] keith-turner opened a new issue, #3254: Comparisons between TserverInstance and TabletMetadata.Location are error prone
keith-turner opened a new issue, #3254:
URL: https://github.com/apache/accumulo/issues/3254
While working on #3251 an equals method was added to [TabletMetadata.Location](https://github.com/apache/accumulo/blob/540179d1f52dcc478eee3a3ee3c5fac106736c8b/core/src/main/java/org/apache/accumulo/core/metadata/schema/TabletMetadata.java#L135-L146) which caused [this test](https://github.com/apache/accumulo/blob/540179d1f52dcc478eee3a3ee3c5fac106736c8b/core/src/test/java/org/apache/accumulo/core/metadata/schema/TabletMetadataTest.java#L199) to break.
The cause of the breakage in the test was [this code](https://github.com/apache/accumulo/blob/540179d1f52dcc478eee3a3ee3c5fac106736c8b/core/src/main/java/org/apache/accumulo/core/metadata/schema/TabletMetadata.java#L293-L294) 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] cshannon commented on issue #3254: Comparisons between TserverInstance and TabletMetadata.Location are error prone
Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon commented on issue #3254:
URL: https://github.com/apache/accumulo/issues/3254#issuecomment-1482707866
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.
https://github.com/apache/accumulo/blob/7d6984200036ee6e120043474387eb981b4bd62d/core/src/main/java/org/apache/accumulo/core/metadata/schema/Ample.java#L260-L262
It would be nicer to have:
```java
TabletMutator putLocation(TabletMetadata.Location location);
TabletMutator deleteLocation(TabletMetadata.Location location);
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on issue #3254: Comparisons between TserverInstance and TabletMetadata.Location are error prone
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on issue #3254:
URL: https://github.com/apache/accumulo/issues/3254#issuecomment-1482775944
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] cshannon closed issue #3254: Comparisons between TserverInstance and TabletMetadata.Location are error prone
Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon closed issue #3254: Comparisons between TserverInstance and TabletMetadata.Location are error prone
URL: https://github.com/apache/accumulo/issues/3254
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] cshannon commented on issue #3254: Comparisons between TserverInstance and TabletMetadata.Location are error prone
Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon commented on issue #3254:
URL: https://github.com/apache/accumulo/issues/3254#issuecomment-1482656617
I also was thinking the same thing that this looked similar to https://github.com/apache/accumulo/issues/2830
I can can ahead and fix it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] cshannon commented on issue #3254: Comparisons between TserverInstance and TabletMetadata.Location are error prone
Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon commented on issue #3254:
URL: https://github.com/apache/accumulo/issues/3254#issuecomment-1482772623
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on issue #3254: Comparisons between TserverInstance and TabletMetadata.Location are error prone
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on issue #3254:
URL: https://github.com/apache/accumulo/issues/3254#issuecomment-1482010168
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on issue #3254: Comparisons between TserverInstance and TabletMetadata.Location are error prone
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on issue #3254:
URL: https://github.com/apache/accumulo/issues/3254#issuecomment-1482780165
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] cshannon commented on issue #3254: Comparisons between TserverInstance and TabletMetadata.Location are error prone
Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon commented on issue #3254:
URL: https://github.com/apache/accumulo/issues/3254#issuecomment-1482777839
> > 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org