You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by "DomGarguilo (via GitHub)" <gi...@apache.org> on 2024/02/08 15:34:43 UTC
[I] Move the test case in AccumuloStoreIT into FateStoreIT [accumulo]
DomGarguilo opened a new issue, #4246:
URL: https://github.com/apache/accumulo/issues/4246
This is a nice test. If possible would be nice to eventually run the test against Zookeeper also like #4202. If it makes sense could be a follow on.
_Originally posted by @keith-turner in https://github.com/apache/accumulo/pull/4160#discussion_r1470246756_
--
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
Re: [I] Move the test case in AccumuloStoreIT into FateStoreIT [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on issue #4246:
URL: https://github.com/apache/accumulo/issues/4246#issuecomment-2253308424
@cshannon - In #4160 the test `testCreateWithCollisionAndExceedRetryLimit` was added. Looking at the new FateStore implementations in `elasticity`, I don't see a retry. Is this issue OBE and can be closed?
--
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
Re: [I] Move the test case in AccumuloStoreIT into FateStoreIT [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on issue #4246:
URL: https://github.com/apache/accumulo/issues/4246#issuecomment-2253389086
Looking at the history of this I found the following.
* This issue was opened about a test the verifies the UserFateStore will retry when the random id it picked for a new fate id exists. It retries up to 5 times and then fails, the test that only exists for UserFateStore checks this.
* The MetaFateStore does not retry and does not have a test for the case of a collision.
* These changes referenced in this issue were made before #4388 which changed the fate id type from 64bit to 128bit. This change dramatically decreases the chance of collision to the point where maybe it should not be expected.
With the change from 64bit to 128bit wondering if instead of implementing and testing the behavior in both stores if we should simplify the expectation to fail on any collision (instead of allowing 5) and test that in both stores.
--
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
Re: [I] Move the test case in AccumuloStoreIT into FateStoreIT [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on issue #4246:
URL: https://github.com/apache/accumulo/issues/4246#issuecomment-2269134542
> The MetaFateStore does not retry and does not have a test for the case of a collision.
I think it does, I think it retries forever at https://github.com/apache/accumulo/blob/elasticity/core/src/main/java/org/apache/accumulo/core/fate/MetaFateStore.java#L92
--
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
Re: [I] Move the test case in AccumuloStoreIT into FateStoreIT [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on issue #4246:
URL: https://github.com/apache/accumulo/issues/4246#issuecomment-2253523584
> What's the implication of a failure here. Does the user have to retry their fate transaction? If so, I'm not sure we want that.
Yeah something would probably blow up. So we could make the MetaFateStore have the same behavior of retrying 5 times and fail and then add the test for 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
Re: [I] Move the test case in AccumuloStoreIT into FateStoreIT [accumulo]
Posted by "cshannon (via GitHub)" <gi...@apache.org>.
cshannon commented on issue #4246:
URL: https://github.com/apache/accumulo/issues/4246#issuecomment-2253351461
> @cshannon - In #4160 the test `testCreateWithCollisionAndExceedRetryLimit` was added. Looking at the new FateStore implementations in `elasticity`, I don't see a retry. Is this issue OBE and can be closed?
I'm not sure, it's been a really long time since I looked at this and this was something Dom added. I took a look and the test still exists in https://github.com/apache/accumulo/blob/elasticity/test/src/main/java/org/apache/accumulo/test/fate/user/UserFateStoreIT.java
--
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
Re: [I] Move the test case in AccumuloStoreIT into FateStoreIT [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on issue #4246:
URL: https://github.com/apache/accumulo/issues/4246#issuecomment-2269018927
> > What's the implication of a failure here. Does the user have to retry their fate transaction? If so, I'm not sure we want that.
>
> Yeah something would probably blow up. So we could make the MetaFateStore have the same behavior of retrying 5 times and fail and then add the test for it.
So, it sounds like in either case the user, or calling code, has to handle the failure. I'm thinking we should either:
1. Make the retry limit infinite (and not throw an exception that has to be handled) if the only reason for a duplicate could be a UUID collision. This is unlikely to happen in the first place, and if it does happen on the first call the probability of it happening again is even smaller, right?
2. Or, just remove the retry entirely and let the user (or calling code) try again.
--
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
Re: [I] Move the test case in AccumuloStoreIT into FateStoreIT [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion closed issue #4246: Move the test case in AccumuloStoreIT into FateStoreIT
URL: https://github.com/apache/accumulo/issues/4246
--
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
Re: [I] Move the test case in AccumuloStoreIT into FateStoreIT [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on issue #4246:
URL: https://github.com/apache/accumulo/issues/4246#issuecomment-2253416593
> With the change from 64bit to 128bit wondering if instead of implementing and testing the behavior in both stores if we should simplify the expectation to fail on any collision (instead of allowing 5) and test that in both stores.
What's the implication of a failure here. Does the user have to retry their fate transaction? If so, I'm not sure we want 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