bharathv edited a comment on pull request #1593: URL: https://github.com/apache/hbase/pull/1593#issuecomment-620918133
Okay, let me first summarize your beef with the current approach, for my own understanding and for those following so that they can weigh in too. Your main concerns are as follows. I'll try to answer each one separately. 1. Implementation is not real async, uses a background driver thread 2. Lack of retries support built into the channel. 3. Implementation doesn't support both blocking and non-blocking RPC implementations 4. Read replicas was not ported to this feature, if we want to go with this approach, port read replicas too (or else you'll veto). 5. HBase client implementation is not like GRPC, so it may not make sense to have hedging support in HBase just because GRPC did it. ===== 1. This is true. May be I was a little lazy and I missed it when I implemented. I quickly put-together what I think is the async version you are looking for. Its here #1601 if wanted to take a look. It doesn't use a dedicated thread anymore and relies on the last failed RPC thread of a previous batch to invoke the next batch in the same thread. Just FYI, the patch is raw and could be cleaned up, I quickly put it together so that you have something to look at. Also, to add, the issue with implementing it as a channel means that we don't have flexibility of returning CompletableFutures (due to proto buf interfaces). That makes the code to look a little unstructured but I think we can back it up with comments. Let me know what you think. 2. I already clarified it in the design and class comments. Hedging is orthogonal to retries. You probably don't want to mix both of them. Also, the implementation adds a "Channel", and it doesn't make sense to have retries at the Channel level. That said, nothing stops us from having retries from the cilent side, just like the your code in AsyncRpcRetryingCaller... 3. I actually disagree with you on this. I don't see why we should support BlockingRpcClient anymore. Async client is the current Java way of doing things and it has feature parity with Blocking client implementation and is the default. Now imagine implementing hedging on a blocking channel, we will have to do all sorts of gymnastics with thread pools (which you probably don't want, given your concern 1). If we end up doing this for a blocking client, it is going to be ton of code (and effort) that no one uses. Do we really want to do that? 4. Well, I don't know how to answer this. I didn't do it because of lack of time. These set of patches were committed probably a month or two back and since then I was busy. I can attempt it but I cannot guarantee that I'll finish it in 2.3.0 time frame given my other commitments. Also, I don't fully understand the concern here. Just because one feature was not ported to a new framework, doesn't mean we should totally get rid of it. 5. Of course HBase is not like GRPC. I was just trying to give an example. The analogy there was that HBase also has an abstraction of rpc stack and client implementation relying on rpc stack. So I think it makes sense to have hedging as a feature in the rpc stack. ---------------------------------------------------------------- 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. For queries about this service, please contact Infrastructure at: [email protected]
