Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12057 )
Change subject: kudu support ttl ...................................................................... Patch Set 1: Thank you for your contribution. I just skimmed this change and had a few questions: 1. Did you intend to publish it against branch-1.7.x and not master? New features should always land in master first, and only be backported to older branches if they're small in scope and well tested. 2. There's very little documentation explaining what the change does. The commit description is non-existent and there are few (if any?) new comments. It's hard to figure out what's going on here. For patches that approach a thousand lines of code we typically like to see, at the very least: 2a. A terse summary of the change. 2b. A detailed description of what has changed. 2c. A discussion of trade-offs, if there were alternatives to consider. 2d. If there are persistent state or wire protocol changes, an evaluation of impact on backwards compatibility (if any). 3. I also see a fair amount of debugging code (new LOG(INFO) messages, #if statements, etc.) in the patch. Did you intend to leave them in? If not, could you clean up the patch in preparation for code review? -- To view, visit http://gerrit.cloudera.org:8080/12057 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: branch-1.7.x Gerrit-MessageType: comment Gerrit-Change-Id: I6503ca90ebca66d154e7084e0c93e94c9b0d9c0c Gerrit-Change-Number: 12057 Gerrit-PatchSet: 1 Gerrit-Owner: YangSong <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Mon, 10 Dec 2018 20:26:59 +0000 Gerrit-HasComments: No
