[GitHub] zookeeper issue #567: ZOOKEEPER-3071: Add a config parameter to control tran...
Github user anmolnar commented on the issue: https://github.com/apache/zookeeper/pull/567 Committed to master branch. Thanks @suyogmapara ! ---
[GitHub] zookeeper issue #567: ZOOKEEPER-3071: Add a config parameter to control tran...
Github user anmolnar commented on the issue: https://github.com/apache/zookeeper/pull/567 @suyogmapara Let's try to get a green build. ---
[GitHub] zookeeper issue #567: ZOOKEEPER-3071: Add a config parameter to control tran...
Github user anmolnar commented on the issue: https://github.com/apache/zookeeper/pull/567 retest this please ---
[GitHub] zookeeper issue #567: ZOOKEEPER-3071: Add a config parameter to control tran...
Github user suyogmapara commented on the issue: https://github.com/apache/zookeeper/pull/567 Hi @anmolnar, do I need to do anything before this can be merged? ---
[GitHub] zookeeper issue #567: ZOOKEEPER-3071: Add a config parameter to control tran...
Github user suyogmapara commented on the issue: https://github.com/apache/zookeeper/pull/567 @tamaashu can you please take a look and approve? ---
[GitHub] zookeeper issue #567: ZOOKEEPER-3071: Add a config parameter to control tran...
Github user asfgit commented on the issue: https://github.com/apache/zookeeper/pull/567 Refer to this link for build results (access rights to CI server needed): https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2478/ ---
[GitHub] zookeeper issue #567: ZOOKEEPER-3071: Add a config parameter to control tran...
Github user tamaashu commented on the issue: https://github.com/apache/zookeeper/pull/567 Hi @suyogmapara, please simply remove your changes from zookeeperAdmin.xml and add them to zookeeperAdmin.md, according to GitHub MarkDown definition. You can also remove the changes from the generated HTML and PDF files I'd docs directory (my onfoing pr removes them). You can test your changes if you do a `mvn clean install` and check the newly generated zookeeper-docs/target/html/zookeeperAdmin.html. ---
[GitHub] zookeeper issue #567: ZOOKEEPER-3071: Add a config parameter to control tran...
Github user suyogmapara commented on the issue: https://github.com/apache/zookeeper/pull/567 Thanks @anmolnar, Hi @tamaashu, can you please let me know how should I move my documentation changes? ---
[GitHub] zookeeper issue #567: ZOOKEEPER-3071: Add a config parameter to control tran...
Github user asfgit commented on the issue: https://github.com/apache/zookeeper/pull/567 Refer to this link for build results (access rights to CI server needed): https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2465/ ---
[GitHub] zookeeper issue #567: ZOOKEEPER-3071: Add a config parameter to control tran...
Github user anmolnar commented on the issue: https://github.com/apache/zookeeper/pull/567 retest this please ---
[GitHub] zookeeper issue #567: ZOOKEEPER-3071: Add a config parameter to control tran...
Github user suyogmapara commented on the issue: https://github.com/apache/zookeeper/pull/567 Hi @anmolnar please take a look at updated documentation. This time after regeneration, other parts of the documentation has changed. I have also rebased on the latest upstream/master ---
[GitHub] zookeeper issue #567: ZOOKEEPER-3071: Add a config parameter to control tran...
Github user asfgit commented on the issue: https://github.com/apache/zookeeper/pull/567 Refer to this link for build results (access rights to CI server needed): https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2462/ ---
[GitHub] zookeeper issue #567: ZOOKEEPER-3071: Add a config parameter to control tran...
Github user anmolnar commented on the issue: https://github.com/apache/zookeeper/pull/567 @suyogmapara Please rebase your commits, because you've merge conflicts. Also please add more detailed documentation to the patch by capturing the most important notes that were mentioned in the pull request. ---
[GitHub] zookeeper issue #567: ZOOKEEPER-3071: Add a config parameter to control tran...
Github user asfgit commented on the issue: https://github.com/apache/zookeeper/pull/567 Refer to this link for build results (access rights to CI server needed): https://builds.apache.org/job/PreCommit-ZOOKEEPER-github-pr-build/2427/ ---
[GitHub] zookeeper issue #567: ZOOKEEPER-3071: Add a config parameter to control tran...
Github user suyogmapara commented on the issue: https://github.com/apache/zookeeper/pull/567 Hi @breed @anmolnar, can you please take a look at this PR? ---
[GitHub] zookeeper issue #567: ZOOKEEPER-3071: Add a config parameter to control tran...
Github user suyogmapara commented on the issue: https://github.com/apache/zookeeper/pull/567 @anmolnar thank you, I will surely update the docs with some more details. I am currently waiting to see if there is anything else I need to address in the next iteration. ---
[GitHub] zookeeper issue #567: ZOOKEEPER-3071: Add a config parameter to control tran...
Github user anmolnar commented on the issue: https://github.com/apache/zookeeper/pull/567 @suyogmapara I tend to accept this patch and understand that the new configuration setting has to be carefully set and should be consistent with `preAllocSize`, otherwise all kind of issues could happen to ZooKeeper. For that very good reason I'd like to ask you to extend the documentation with a lot more information about it. Everything that you mentioned here including the examples should be added to doc with highlighting how careful the user should be when touching it. @breed Do you accept the patch? ---
[GitHub] zookeeper issue #567: ZOOKEEPER-3071: Add a config parameter to control tran...
Github user suyogmapara commented on the issue: https://github.com/apache/zookeeper/pull/567 Hi @maoling Regarding 2, I agree that this would be the outcome. But sorry it is not clear to me why do you think that the result is surprising. You are setting log-size-limit slightly larger than the pre-alloc size and so you would see that actual log size is twice the preAllocSize. Regarding 1, it seems to me that we configured zookeeper to perform poorly and so it would do poorly. It is similar to setting snapCount = 1.As I said above, I can explicitly prevent this case if you guys think such a validation is useful. As a side note, example of a good config would be preAllocSize = 64 MB, txnLogSizeLimit = 512 MB. Regarding 3, 1 MB is a quite large and typical configuration of snapCount would mean we will have txnLogs of several GBs. Regarding 4, txnLogSizeLimitInKb controls maximum size of the transaction log. I believe there is a separate parameter that controls batch commit size on observers. ---
[GitHub] zookeeper issue #567: ZOOKEEPER-3071: Add a config parameter to control tran...
Github user maoling commented on the issue: https://github.com/apache/zookeeper/pull/567 @suyogmapara - **1** just as we both observe that when `txnLogSizeLimitInKb < preAllocSize`,for every transaction,it will create a log block. -1 - **2** when set txnLogSizeLimitInKb > preAllocSize e.g `"-Dzookeeper.txnLogSizeLimitInKb=25600" "-Dzookeeper.preAllocSize=20480"` ` ./zk-latencies.py --servers "localhost:2181" --root_znode="/zk-latencies6" --znode_count=1 --znode_size=1024 --synchronous` crete nodes( with size:1024byte--1kb) ceaselessly,we will see the log block will rise from 20971536 byte to 41943056 byte. - **3**.since one node size has been limited 1MB,So it has decided that a single transaction log will not be very large? - **4**.look at the code,the `txnLogSizeLimitInKb` controls the size of a single transaction log? or the size of a group commit?the control of this size is not accurate? ---
[GitHub] zookeeper issue #567: ZOOKEEPER-3071: Add a config parameter to control tran...
Github user suyogmapara commented on the issue: https://github.com/apache/zookeeper/pull/567 Hi @maoling I tested the patch with following parameters: 1. PreAllocSize = 100 KB, TxnLogSizeLimit = 200 KB, 100 transactions of little over 10 KBs. Outcome: 5 transaction files with little over 200 KBs each (rounded up to last transaction). 2. PreAllocSize = 100 KB, TxnLogSizeLimit = 100 KB, 100 transactions of little over 10 KBs. Outcome: 100 transaction files one per each transaction 3. PreAllocSize = 100 KB, TxnLogSizeLimit = 50 KB, 100 transactions of little over 10 KBs. Outcome: 100 transaction files one per each transaction. Second and third outcomes are expected as every time it tries to pre-allocate more, we would be rolling the log. Of course those won't be sensible configurations, but if someone uses them, only performance would be impacted and not the correctness. Please let me know if you disagree. @anmolnar snapCount doesn't give direct handle on the transaction log size. Changing average transaction size from 1 KB to 100 KB would increase the transaction log size by 2 orders of magnitude and may push they sync time over the edge. More importantly reducing the snapCount would also increase how frequently we snapshot and so we would be forced to find a sweet spot to balance the two conflicting goals. On the other hand, txnLogSizeLimit provides cheap and independent control on the worst case seek time during the diff sync. ---
[GitHub] zookeeper issue #567: ZOOKEEPER-3071: Add a config parameter to control tran...
Github user anmolnar commented on the issue: https://github.com/apache/zookeeper/pull/567 @suyogmapara I think @maoling 's concerns are valid. I also would like to get some more insight on what value would this patch add to control / limit the size of txn log files. We already have the `snapCount` parameter, why is it not enough? When catching up the follower, leader is able to filter log files by checking the last stored zxid in their names and you can control the maximum seek time within log files by adjusting the `snapCount` parameter. What am I missing? ---
[GitHub] zookeeper issue #567: ZOOKEEPER-3071: Add a config parameter to control tran...
Github user suyogmapara commented on the issue: https://github.com/apache/zookeeper/pull/567 Hi @maoling Regarding 1, preAllocSize controls how big the contiguous region of transaction file would be on disk (to reduce number seeks you need to do) while this feature controls how big the file size would be before we roll to the next transaction log. It would make sense to set later one as some multiple of the first one. If you set both values closers or if preAllocSize is smaller, you are right you would get very frequent log rolls (maybe for every transaction in 1.2), but as far as I can think of it shouldn't impact correctness. I don't have checks against such values mainly because it is not clear what is the minimum value I should allow. Basically the same reason we don't have check against setting snapCount = 2, for example. Regarding your example, can you please elaborate your values in 1.1? Regarding 2, during diff sync, leader will seek for the last transaction follower has seen in the in-memory committed log cache. If the zxid is older than oldest transaction then leader has to first send over the transactions from the earlier transaction log file. For starting the stream of transaction, leader has to do a linear search for the zxid, in the transaction log (as we don't have an index over it). Larger the transaction log file, longer it takes. It can happen if you have especially larger individual transactions. This feature give you control over the maximum size. ---
[GitHub] zookeeper issue #567: ZOOKEEPER-3071: Add a config parameter to control tran...
Github user maoling commented on the issue: https://github.com/apache/zookeeper/pull/567 @suyogmapara - **1**.suspicious of the behavious of the method `getCurrentLogSize` when I test this patch: - **1.1** set txnLogSizeLimitInKb > preAllocSize e.g `"-Dzookeeper.txnLogSizeLimitInKb=25600" "-Dzookeeper.preAllocSize=20480"` the log block will rise from `20971536` byte to `41943056` byte.it has changed the logic about preAllocSize and roll over? - **1.2** set txnLogSizeLimitInKb < preAllocSize e.g `"-Dzookeeper.txnLogSizeLimitInKb=10240" "-Dzookeeper.preAllocSize=20480"` it will increase the number of log blocks insanely - **2**.still not very clear about what the parameter: `txnLogSizeLimitInKb` wants to controll and the advantage. Could you plz explain the following for me? when this happens? > This is because leader has to scan through the appropriate log file on disk to find the transaction to start sync from. ---
[GitHub] zookeeper issue #567: ZOOKEEPER-3071: Add a config parameter to control tran...
Github user anmolnar commented on the issue: https://github.com/apache/zookeeper/pull/567 @suyogmapara Thanks, that makes perfect sense. Please address the build issues and we're ready to commit. ---
[GitHub] zookeeper issue #567: ZOOKEEPER-3071: Add a config parameter to control tran...
Github user suyogmapara commented on the issue: https://github.com/apache/zookeeper/pull/567 Thanks @anmolnar for the review. Regarding randomization, I think they did it for snapCount because snapshot is potentially expensive IO and entire ensemble doing it at the same time may be an issue. This feature doesn't do snapshot when we roll the log and just switches the file we are writing to. So it may be okay if everyone do it at the same time. Please let me know if you disagree. ---
[GitHub] zookeeper issue #567: ZOOKEEPER-3071: Add a config parameter to control tran...
Github user anmolnar commented on the issue: https://github.com/apache/zookeeper/pull/567 @suyogmapara Thanks for adding the documentation, it's very useful. I think it was a very good idea to put it next to `snapCount`. There's one thing however which caught my eye from snapCount doc: > In order to prevent all of the machines in the quorum from taking a snapshot at the same time, each ZooKeeper server will take a snapshot when the number of transactions in the transaction log reaches a runtime generated random value in the [snapCount/2+1, snapCount] range.The default snapCount is 100,000. Isn't `txnLogSizeLimitInKb` affected by the same issue? Do you think it would be useful to implement something similar for the new option as well? ---
[GitHub] zookeeper issue #567: ZOOKEEPER-3071: Add a config parameter to control tran...
Github user suyogmapara commented on the issue: https://github.com/apache/zookeeper/pull/567 @anmolnar I updated the documentation, please let me know if it looks alright ---
[GitHub] zookeeper issue #567: ZOOKEEPER-3071: Add a config parameter to control tran...
Github user suyogmapara commented on the issue: https://github.com/apache/zookeeper/pull/567 @anmholnar Sorry I got caught up in other work, I will update this PR with the documents today. ---
[GitHub] zookeeper issue #567: ZOOKEEPER-3071: Add a config parameter to control tran...
Github user anmolnar commented on the issue: https://github.com/apache/zookeeper/pull/567 I still miss the documentation change here and I think it would be better to commit it together with the code change, but I'm happy to approve it if you create a separate Jira for that. ---
[GitHub] zookeeper issue #567: ZOOKEEPER-3071: Add a config parameter to control tran...
Github user anmolnar commented on the issue: https://github.com/apache/zookeeper/pull/567 @suyogmapara Would you please trigger the Jenkins build? @maoling @breed Do approve this commit? ---
[GitHub] zookeeper issue #567: ZOOKEEPER-3071: Add a config parameter to control tran...
Github user suyogmapara commented on the issue: https://github.com/apache/zookeeper/pull/567 @maoling Thanks for the review, regarding concern around PreAllocSize, thanks to ZOOKEEPER-2249, setting txnLogSizeLimit less than preAllocSize should not cause any issue. I can add explicit test for it if you guys think it is valuable. ---
[GitHub] zookeeper issue #567: ZOOKEEPER-3071: Add a config parameter to control tran...
Github user suyogmapara commented on the issue: https://github.com/apache/zookeeper/pull/567 Thanks for the review @anmolnar. Please take a look at unit tests when you get a chance. I have addressed the comments. I will also add documentation in a separate commit. ---
