<HTML><BODY><div><br>Sergey, thanks a lot for the review.<blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;">1. sync() can be asynchonous operation so we should wait it or somehow make<br>sure it has finished.<br>7. sync() can be asynchonous operation, we should wait it to make sure<br>it has finished.<br>17. the same comment as above regarding sync()</blockquote></div><div>The «sync» behaviour can be asynchronous, however, modern Linux has been waiting for the recording to finish.<br>Any way I will add checking exit status from sync.</div><div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;">2. What does '500' mean? Let's move magic numbers to constants.</blockquote></div><div>Sure. Will fix it.</div><div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;">3. As far as I understand cbench_runner.lua wrote two files<br>cbench_output_memtx_write.txt and cbench_output_vinyl_write.txt, you<br>pass filenames as arguments. On next step we should extract interested<br>numbers from these files. But cbench_output_memtx_write.txt is unused<br>here. And how other files were created: cbench_output_memtx.txt and<br>cbench_output_vinyl_fsync.txt?</blockquote></div><div>There are 3 runs for cbench with different parameters which produce 3 output files:</div><div>cbench_output_memtx.txt</div><div>cbench_output_vinyl_fsync.txt</div><div>cbench_output_vinyl_write.txt</div><div>which are parsed to 4 results files.</div><div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;">4. Lines with grep/sed looks absolutely unreadable. Please split lines, like below:<br> grep "^?tab" cbench_output_memtx.txt |<br> sed "s/.*name=//"| sed "s/¶m=/:/"| sed "s/cb\./cb\.memtx\./" |<br> tee -a cbench_result.txt<br> grep "^?tab" cbench_output_vinyl_fsync.txt |<br> sed "s/.*name=//"| sed "s/¶m=/:/"| sed "s/cb\./cb\.vinyl\.fsync\./"|<br> tee -a cbench_result.txt<br>6. set -u<br>9. It is not clear why filemask includes zero at the beginning.<br>10. please remove commented lines or uncomment them<div>12. you can implement the same body of function much simpler:</div><div> with open(filename) as raw_data:<br> return raw_data.readlines()<br>13. you can implement the same body of function much simpler:</div><div> with open(filename) as raw_data:<br> return raw_data.readlines()[-1][0]</div>14. Deviations<br>16. set -u<br>17(b). filemask for *.xlog and *.snap</blockquote></div><div>Refactoring. I’ve added changes you suggested.</div><div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;">5. Running of publish.py will broke if we will move repo to another<br> directory. Let's use relative path to publish.py.<br>8. Running of publish.py will broke if we will move repo to another<br> directory. Let's use relative path to publish.py.<br>11. Running of publish.py will broke if we will move repo to another<br> directory. Let's use relative path to publish.py.<br>15. Running of publish.py will broke if we will move repo to another<br> directory. Let's use relative path to publish.py.<br>18. Running of publish.py will broke if we will move repo to another<br> directory. Let's use relative path to publish.py.<br>21. Running of publish.py will broke if we will move repo to another<br>directory. Let's use relative path to publish.py.</blockquote></div><div>This absolute path from container root. As we agreed it will be kept.</div><div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;">19. cbench script uses physcpubind equal to 1, why we set exact these<br>numbers and why physcpubind has not the same set of numbers?</blockquote></div><div>For performance testing we are using servers with 2 processors</div><div>and each processor have few (6) cores. For stable results we bind tasks</div><div>to single proc or even single core. The difference can be because</div><div>of bench requirement (as Tarantool can run multithread execution).</div><div>So this is still open question how to do correctly binding, but for now</div><div>I would suggest to keep this parameters unchanged. </div><div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;">20. What does "l" variable mean?</blockquote></div><div>This variables changes in range (for l in a b c d e f ; do) and runs different workloads:</div><div> -rw-r--r-- 1 root root 3014 Apr 24 07:24 workloada<br> -rw-r--r-- 1 root root 3050 Apr 24 07:24 workloadb<br> -rw-r--r-- 1 root root 3036 Apr 24 07:24 workloadc<br> -rw-r--r-- 1 root root 3399 Apr 24 07:24 workloadd<br> -rw-r--r-- 1 root root 3558 Apr 24 07:24 workloade<br> -rw-r--r-- 1 root root 3149 Apr 24 07:24 workloadf</div><div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;">22. Why do we need gperftools here?</blockquote>Possible artefact from debugging phase a long time ago. Doesn't need for perf testing anymore. Removing.<div> </div><div data-signature-widget="container"><div data-signature-widget="content"><div>--<br>Oleg Piskunov</div></div></div><div> </div></div></BODY></HTML>