[GitHub] eolivelli commented on issue #1436: BP-14 force() API - client side implementation
eolivelli commented on issue #1436: BP-14 force() API - client side implementation URL: https://github.com/apache/bookkeeper/pull/1436#issuecomment-397652776 rerun bookkeeper-server bookie tests This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] eolivelli commented on issue #1436: BP-14 force() API - client side implementation
eolivelli commented on issue #1436: BP-14 force() API - client side implementation URL: https://github.com/apache/bookkeeper/pull/1436#issuecomment-397637455 rerun bookkeeper-server bookie tests This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] eolivelli commented on issue #1436: BP-14 force() API - client side implementation
eolivelli commented on issue #1436: BP-14 force() API - client side implementation URL: https://github.com/apache/bookkeeper/pull/1436#issuecomment-397612674 rerun bookkeeper-server bookie tests This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] eolivelli commented on issue #1436: BP-14 force() API - client side implementation
eolivelli commented on issue #1436: BP-14 force() API - client side implementation URL: https://github.com/apache/bookkeeper/pull/1436#issuecomment-397552150 rerun bookkeeper-server bookie tests This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] eolivelli commented on issue #1436: BP-14 force() API - client side implementation
eolivelli commented on issue #1436: BP-14 force() API - client side implementation URL: https://github.com/apache/bookkeeper/pull/1436#issuecomment-397527391 @jvrao @sijie @jiazhai Thank you ! I have rebased to latest master, I will wait for CI to pass and then merge. This was the last important part of BP-14 ! I will follow up with docs I think existing BC tests are enough to guarantee that we did not break compatibility. Now that BP-14 has got its final form I will start tests and benchmarks on downstream applications (once this change reaches Apache Snapshots Repository) This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] eolivelli commented on issue #1436: BP-14 force() API - client side implementation
eolivelli commented on issue #1436: BP-14 force() API - client side implementation URL: https://github.com/apache/bookkeeper/pull/1436#issuecomment-396881301 @sijie @jvrao FYI I have added a commit to drop 'error out pending adds" in case of failure in force operation. So we are *not* erroring out pending adds, they will fail naturally in case of bookie failure. a force() which fails simply does not advance LastAddConfirmed. Hopefully @sijie and @jiazhai will be still okay with the patch This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] eolivelli commented on issue #1436: BP-14 force() API - client side implementation
eolivelli commented on issue #1436: BP-14 force() API - client side implementation URL: https://github.com/apache/bookkeeper/pull/1436#issuecomment-396571250 I meant @jvrao suggested to error our pending adds. I think we do not need to do so. I will remove that from the patch, but before spending time I would like to have acknowledgement feom JV This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] eolivelli commented on issue #1436: BP-14 force() API - client side implementation
eolivelli commented on issue #1436: BP-14 force() API - client side implementation URL: https://github.com/apache/bookkeeper/pull/1436#issuecomment-396482039 @sijie in the fsync / force metaphor I think it is better not to fail pending adds. It was a comment/request from @jvrao. I am going to drop that op, but I need to ask to JV This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] eolivelli commented on issue #1436: BP-14 force() API - client side implementation
eolivelli commented on issue #1436: BP-14 force() API - client side implementation URL: https://github.com/apache/bookkeeper/pull/1436#issuecomment-396160571 @sijie @jvrao waiting for your comments and hopefully approval. I have found an edge case, but I don't know if it is worth handling it: - we are erroring out pending adds in case of failure of a force - we are NOT erroring out pending "force" operations in case of write failures I this that anyway if any add fails, as we do not allow ensemble changes, the "force" will eventually fail This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] eolivelli commented on issue #1436: BP-14 force() API - client side implementation
eolivelli commented on issue #1436: BP-14 force() API - client side implementation URL: https://github.com/apache/bookkeeper/pull/1436#issuecomment-395124763 @jvrao @sijie All of your comments addressed. Now it looks really better ! Thank you Additional implementation to note: **Error out** all pending adds in case of failure of **force()** This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] eolivelli commented on issue #1436: BP-14 force() API - client side implementation
eolivelli commented on issue #1436: BP-14 force() API - client side implementation URL: https://github.com/apache/bookkeeper/pull/1436#issuecomment-394740832 retest this please This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] eolivelli commented on issue #1436: BP-14 force() API - client side implementation
eolivelli commented on issue #1436: BP-14 force() API - client side implementation URL: https://github.com/apache/bookkeeper/pull/1436#issuecomment-393634809 @jvrao @jiazhai please take a new look as you have cycles. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] eolivelli commented on issue #1436: BP-14 force() API - client side implementation
eolivelli commented on issue #1436: BP-14 force() API - client side implementation URL: https://github.com/apache/bookkeeper/pull/1436#issuecomment-393094971 @sijie In the limited scope of this PR I think that it is good that we are not breaking ExplicitLAC and it is working as expected (I have added a test case as proof). I think that in general, despite BP14, the fact that LAC is only piggybacked on writes (and written finally on close/recovery...) is very annoying for tailing readers. The best would be to enable ExplicitLAC by default but as far as I can see it is not fully integrated in the normal flow of a tailing reader (you have to explicitly read ExplicitLAC from the reader side). In my opinion it is better for us to keep ExplicitLAC RPC as it is now. We have choosen to keep force RPC away from ledger storage (no masterKey on wireprotocol), but in the future we can add an optional payload with masterKey and current client-side LAC. We could make force() to really write a meta entry which contains the LAC and integrate this in the story and so ExplicitLAC won't be necessary anymore if we perform force() in background, even for non-DEFERRED_SYNC writers. So a recap for this PR and my view of the story after BP14: - the story we are writing is healthy and it does not break existing assumptions - it opens the door to future enhancements, and maybe to the deprecation of ExplicitLAC, in favour of standard LAC (we are introducing a mechanism to let LAC advance on writer side without "writes") I will be happy to continue personally this work and make BK more like a storage system cc @jvrao This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] eolivelli commented on issue #1436: BP-14 force() API - client side implementation
eolivelli commented on issue #1436: BP-14 force() API - client side implementation URL: https://github.com/apache/bookkeeper/pull/1436#issuecomment-393079231 @sijie I agree at 100% with your higher level description. I would like to have such "background flush" . I am looking deeply into ExplicitLAC, and my feeling is that it will need to be reworked a bit in order to make it usable for this purpose. It *seems* to me that ExplicitLAC is not integrated with the usual workflow: if you only configure SetExplicitLACInterval LAC is not advancing automagically. It would be lovely to have a new "background flush" mechanism which performs a background "force()" and then makes LAC advance without writes, like ExplicitLAC. Maybe the best approach would be to open a new followup task to enhance and rework ExplicitLAC. cc @jvrao This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] eolivelli commented on issue #1436: BP-14 force() API - client side implementation
eolivelli commented on issue #1436: BP-14 force() API - client side implementation URL: https://github.com/apache/bookkeeper/pull/1436#issuecomment-392909847 @sijie your point is very interesting. Let me think about it a bit more. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] eolivelli commented on issue #1436: BP-14 force() API - client side implementation
eolivelli commented on issue #1436: BP-14 force() API - client side implementation URL: https://github.com/apache/bookkeeper/pull/1436#issuecomment-392801953 @sijie I have added a test case about using ExplicitLAC and DEFERRED_SYNC. I think that current behaviour is what I am expecting: 1) LastAddConfirmed advances only in case of force() 2) ExplicitLAC timer sends notifications only when LastAddConfirmed, in case of DEFERRED_SYNC this is possible only after force() 3) without force() ExplicitLAC does nothing, as expected. It seems to me that this is consistent with the expectations of DEFERRED_SYNC writeflag. > is it better to extend the explicit lac request with a flag that force flushing the data? In that way the behavior would be better defined when people enable explicit lac on a deferred-sync ledger. I think that the client controls LAC using force() and ExplicitLAC is a background task which seems to me like an optimization. I don't see much benefit in having an "implicit" force with the ExplicitLAC notification. Maybe it is like the initial proposal of having a "piggybacked LastAddPersisted" on AddResponse which would have made LAC to advance even without an explicit "force()". We could add a flag which enables a timer to send explicit force() periodically, or that performs a force() during the execution of ExplicitLAC timer. I prefer to keep things separated at wire protocol, we can compose the two RPCs, they are lightweight and we are in a background task (the ExplicitLAC timer) cc @jvrao This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] eolivelli commented on issue #1436: BP-14 force() API - client side implementation
eolivelli commented on issue #1436: BP-14 force() API - client side implementation URL: https://github.com/apache/bookkeeper/pull/1436#issuecomment-392798320 @jiazhai @jvrao I have addressed all of your comments and provided answers. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] eolivelli commented on issue #1436: BP-14 force() API - client side implementation
eolivelli commented on issue #1436: BP-14 force() API - client side implementation URL: https://github.com/apache/bookkeeper/pull/1436#issuecomment-392650536 @sijie explicitlac for deferred sync writers only sends the value of LastAddConfirmed and it is still a way to notify LAC changes without a real write. The difference is that LastAddConfirmed variable does not advance at every write. I think they are different operations, and need different semantics. Force() must be acknowledged by the full ensemble. You can use force() without sending explicitlac, in fact new LAC will be piggybacked at next write for instance. I think it is better to keep force and explicitlac separated. I will add test cases to cover the usage of explicit LAC with deferred sync flag. I will check the implementation today, in case I am missing something. Cc @jvrao This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] eolivelli commented on issue #1436: BP-14 force() API - client side implementation
eolivelli commented on issue #1436: BP-14 force() API - client side implementation URL: https://github.com/apache/bookkeeper/pull/1436#issuecomment-392555446 retest this please This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] eolivelli commented on issue #1436: BP-14 force() API - client side implementation
eolivelli commented on issue #1436: BP-14 force() API - client side implementation URL: https://github.com/apache/bookkeeper/pull/1436#issuecomment-392429868 retest this please This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] eolivelli commented on issue #1436: BP-14 force() API - client side implementation
eolivelli commented on issue #1436: BP-14 force() API - client side implementation URL: https://github.com/apache/bookkeeper/pull/1436#issuecomment-391756742 This is still WIP but almost complete, missing items: - prevent v2 clients to use force() API - add test cases for corner cases - disable ensemble changes with DEFERRED_SYNC (this can be another patch) @jvrao @sijie if you have cycles you can start your review This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] eolivelli commented on issue #1436: BP-14 force() API - client side implementation
eolivelli commented on issue #1436: BP-14 force() API - client side implementation URL: https://github.com/apache/bookkeeper/pull/1436#issuecomment-392336831 @jvrao @sijie the patch is ready for review now This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
