[GitHub] sijie commented on issue #970: ISSUE #966: Expose quorum write complete latency to the client
sijie commented on issue #970: ISSUE #966: Expose quorum write complete latency to the client URL: https://github.com/apache/bookkeeper/pull/970#issuecomment-366501006 all CI job passed except integration tests. the integration tests seems to fail due to can't connect to zookeeper. it seems irrelevant to this change. so ignore integration test ci to merge this change. 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] sijie commented on issue #970: ISSUE #966: Expose quorum write complete latency to the client
sijie commented on issue #970: ISSUE #966: Expose quorum write complete latency to the client URL: https://github.com/apache/bookkeeper/pull/970#issuecomment-366501014 IGNORE CI 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] sijie commented on issue #970: ISSUE #966: Expose quorum write complete latency to the client
sijie commented on issue #970: ISSUE #966: Expose quorum write complete latency to the client URL: https://github.com/apache/bookkeeper/pull/970#issuecomment-366421776 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] sijie commented on issue #970: ISSUE #966: Expose quorum write complete latency to the client
sijie commented on issue #970: ISSUE #966: Expose quorum write complete latency to the client URL: https://github.com/apache/bookkeeper/pull/970#issuecomment-364621467 ping @eolivelli 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] sijie commented on issue #970: ISSUE #966: Expose quorum write complete latency to the client
sijie commented on issue #970: ISSUE #966: Expose quorum write complete latency to the client URL: https://github.com/apache/bookkeeper/pull/970#issuecomment-362750216 @eolivelli can you review this again? @athanatos addressed your comment. 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] sijie commented on issue #970: ISSUE #966: Expose quorum write complete latency to the client
sijie commented on issue #970: ISSUE #966: Expose quorum write complete latency to the client URL: https://github.com/apache/bookkeeper/pull/970#issuecomment-360240280 @athanatos do you mind rebasing your PR to latest master? I think it is ready to go once @ivankelly approves it. 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] sijie commented on issue #970: ISSUE #966: Expose quorum write complete latency to the client
sijie commented on issue #970: ISSUE #966: Expose quorum write complete latency to the client URL: https://github.com/apache/bookkeeper/pull/970#issuecomment-359091916 sorry I missed this one. I approved this one. so I need another +1 from other committer to get this merged. @jiazhai @merlimat @ivankelly @eolivelli @jvrao can anyone take a look and ship this ? 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] sijie commented on issue #970: ISSUE #966: Expose quorum write complete latency to the client
sijie commented on issue #970: ISSUE #966: Expose quorum write complete latency to the client URL: https://github.com/apache/bookkeeper/pull/970#issuecomment-357389387 @athanatos okay for me 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] sijie commented on issue #970: ISSUE #966: Expose quorum write complete latency to the client
sijie commented on issue #970: ISSUE #966: Expose quorum write complete latency to the client URL: https://github.com/apache/bookkeeper/pull/970#issuecomment-357357851 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] sijie commented on issue #970: ISSUE #966: Expose quorum write complete latency to the client
sijie commented on issue #970: ISSUE #966: Expose quorum write complete latency to the client URL: https://github.com/apache/bookkeeper/pull/970#issuecomment-357357514 @athanatos > If we go in that direction, would it be better to modify the above patch to pass the same WriteResult object rather than adding an argument? I think it is fine to have current approach, since you guys might still need this old client for BC purpose. We can create an issue for adding this feature to the new client api. > It would be a natural place to add any future information we want to pass out of the write pipeline. yes. > I think the main disadvantage would be yet another allocation? I think this can be addressed with object pooling. for example, https://github.com/apache/bookkeeper/blob/master/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/impl/LedgerEntriesImpl.java 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
