Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/10674 )
Change subject: KUDU-2441 - [python] Enable configuration of mutation buffer ...................................................................... Patch Set 4: (8 comments) http://gerrit.cloudera.org:8080/#/c/10674/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/10674/4//COMMIT_MSG@7 PS4, Line 7: KUDU-2441 - [python] nit: Could you order like "[<topic area>] KUDU-XXXX"? Sorry to be a nit on this, but the consistency helps me scan a git log --oneline. http://gerrit.cloudera.org:8080/#/c/10674/4//COMMIT_MSG@11 PS4, Line 11: resepect respect http://gerrit.cloudera.org:8080/#/c/10674/4/python/kudu/client.pyx File python/kudu/client.pyx: http://gerrit.cloudera.org:8080/#/c/10674/4/python/kudu/client.pyx@528 PS4, Line 528: mutation_buffer_sz=None, : mutation_buffer_watermark=None, mutation_buffer_flush_interval=None, : mutation_buffer_max_num=None I'm not sure what's idiomatic in Python, and I think it could reasonably differ because of Python's named arguments, but should we consider packing these related config values in an Options struct or whatever is comparable in Python? Session could acquire a lot of named parameters in the future. We could also opt to provide only setters and no params here. http://gerrit.cloudera.org:8080/#/c/10674/4/python/kudu/client.pyx@540 PS4, Line 540: Size of the buffer space to set (number of bytes) nit: "Size in bytes of the ..." and remove the parenthetical. http://gerrit.cloudera.org:8080/#/c/10674/4/python/kudu/client.pyx@542 PS4, Line 542: The duration of the interval for the time-based : flushing, in milliseconds. This means that a flush will happen at least once in every interval of time of duration at least 'mutation_buffer_flush_interval'? http://gerrit.cloudera.org:8080/#/c/10674/4/python/kudu/client.pyx@1272 PS4, Line 1272: nit: Add a colon here and for the other flush modes. http://gerrit.cloudera.org:8080/#/c/10674/4/python/kudu/client.pyx@1295 PS4, Line 1295: to to set to http://gerrit.cloudera.org:8080/#/c/10674/4/python/kudu/client.pyx@1295 PS4, Line 1295: 50% 50% of the buffer space? -- To view, visit http://gerrit.cloudera.org:8080/10674 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I52ac48e7dddc31e666a95ace4c7672da51d80b11 Gerrit-Change-Number: 10674 Gerrit-PatchSet: 4 Gerrit-Owner: Jordan Birdsell <[email protected]> Gerrit-Reviewer: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Reviewer: Will Berkeley <[email protected]> Gerrit-Comment-Date: Tue, 26 Jun 2018 18:21:29 +0000 Gerrit-HasComments: Yes
