Move the test case in AccumuloStoreIT into FateStoreIT

[DomGarguilo]

In #4160 an IT was added to test some of the conditions that were added to the conditional mutations. As mentioned below, it would be nice to move this test case to also run against zookeeper.

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 #4160 (comment)

[dlmarion]

@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?

[cshannon]

@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

So it seems like there's still a retry being tested

[keith-turner]

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 FateId to use UUID instead of long #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.

[dlmarion]

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.

[keith-turner]

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.

[dlmarion]

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.

[dlmarion]

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