pkuwm commented on pull request #1119: URL: https://github.com/apache/helix/pull/1119#issuecomment-650362227
> > > You mentioned the sync is not in the event thread, but it is in your code. Or I missed anything? > > > sync() can be waited by using the callback. Do we want to do it? Or if it fails, how we ensure the following read is valid? > > > > > > I mean we don't call it in **Zookeeper client event thread** or where process() is called. (Note Zookeeper client has send thread). It is invoked in zkclient event thread as usual. > > sync() call is actually async. The question is that should we wait for the async sync() finishing before proceeding. So there are two possibilities. > > 1/ sync() call arrived server, but response back to client is lost. > > 2/ sync() call did not arrive at server. > > If it arrives at server, even response is lost, it is ok. We achieved what we want. This the reason why they only provide an async version of sync() I guess. They don't see the need for it block further processing, such as in Zkclient event thread we use here. > > If it did not arrive at server, it would be ideal if there is an exception thrown. But they don't. > > Overall, we can wait for the sync() till it returned successful, retry connectionloss KeeperException only. (Not need to retry session expiration). This would block the Zkclient event thread further and mostly likely unnecessary. (Or why Zookeeper don't provide a sync version of sync()?) > > So for the choice of either wait for sync() or not, I just choose not wait for it. The con of this choice is that there is a small chance it may not arrive at server (due to connection loss) and later zk read did succeed after connection recover without session expiration. > > @jiajunwang, @pkuwm, let me know what is your take here. > > I prefer to wait for the callback. When handling new session, taking more time is ok. The thing is that this is to prevent a corner case if we are not 100% sure, then this fix is compromised I think. @kaisun2000 I prefer to make sure sync is successful, because it could fail. If it fails, we still continue handling new session, it does not achieve our expectation that the data we read is actually synced/new. And @jiajunwang mentioned that taking more time to handle new session is fine. Then I don't see no reason to check the success of sync. Then my question is, if sync fails, shall we continue? I understand we could retry, but what if it still fails? ---------------------------------------------------------------- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
