[GitHub] [accumulo] ctubbsii commented on pull request #2967: Update the new modify properties API to automatically retry on concurrent modification exception.

[GitBox <gi...@apache.org>]

ctubbsii commented on PR #2967:
URL: https://github.com/apache/accumulo/pull/2967#issuecomment-1264531146

   Thoughts on those experiments (skip over for my conclusion):
   
   * I'm not sure it's necessary to name the method "Async()" when it returns a Future. I think that's implied. But, I suppose it doesn't hurt to be explicit. I just wouldn't want to have two versions.... one async and one synchronous, because we imply it with the name suffix, because that name suffix is often used to distinguish between two versions of the same command in many other APIs (ZooKeeper does this, for one example).
   * I agree returning the final map is nice (very convenient)
   * I'm not sure the existing retry forever is the same kind of indefinite retry as we've been discussing. That retries trying to communicate with the server. This retry is a new retry when the server communicates fine, but communicates the detection of a collision. I don't think they're comparable.
   * I don't think it's a problem to use the ForkJoinPool as a default, but I think it's a bit of API bloat to start accepting executors on all async APIs. I think if we start adding async APIs, we should default to the ForkJoinPool, and provide some other mechanism to set an executor at the time the AccumuloClient is constructed, for use with that client, and shut down when that client is closed. This goes back to previous discussions about having per-client resources, and possibly making those resources sharable across clients only in explicit ways at the time the client is constructed. There shouldn't be any global JVM resources being auto-magically created and used and cleaned up (think SingletonManager stuffs).
   * You started using the term "conditional updates" in the method names of the test code. While this method can certainly be used for conditional updates, and the tests themselves may be doing conditional updating, I just want to be clear that this feature itself is not necessarily a conditional update API, because we do not impose any conditional gating. Any conditionals would be imposed by the user in their own mutator code. I mention this only because I think we should be careful not to use the term "conditional" as a general term for this feature. I don't think you were doing that... but it's something that might easily be done if we're not careful, because we have ConditionalMutation, which is very similar, for user data. So, if we're not careful, it could get confusing for users.
   
   My conclusion: Based on the experimentation you've done, and the state of this discussion, and thinking about how much work is still needed to design asynchronous APIs in a way that's consistent going forward, instead of just doing it as a one-off for this specific feature, I'm thinking that I'd prefer to settle with the infinite retry you initially proposed, and to punt on the asynchronous stuff until 3.x, so we can do it consistently.


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