Abhishek Chennaka has posted comments on this change. ( http://gerrit.cloudera.org:8080/18712 )
Change subject: KUDU-2671 Make WebUI compatible with custom hash schema ...................................................................... Patch Set 9: (9 comments) http://gerrit.cloudera.org:8080/#/c/18712/8/src/kudu/common/partition.cc File src/kudu/common/partition.cc: http://gerrit.cloudera.org:8080/#/c/18712/8/src/kudu/common/partition.cc@1039 PS8, Line 1039: > nit: move this closer to line 1048 where it's used? Done http://gerrit.cloudera.org:8080/#/c/18712/8/src/kudu/common/partition.cc@1040 PS8, Line 1040: // Decode the lower and upper bounds > nit: move this closer to line 1070 where it's used? The DecodeRangeKey() below is setting this to remainder of the composite key after decoding, so had to use it before this is called. http://gerrit.cloudera.org:8080/#/c/18712/8/src/kudu/common/partition.cc@1071 PS8, Line 1071: .appen > style nit: stick the ampersand to the type, not to the variable Done http://gerrit.cloudera.org:8080/#/c/18712/8/src/kudu/common/partition.cc@1215 PS8, Line 1215: : else { : entry.append("<td> > Semantically, this should always be if/else regardless of the 'entry' empti Agree, it should have been updated after we revised the loop from linear structure. Fixed it now http://gerrit.cloudera.org:8080/#/c/18712/8/src/kudu/master/master-test.cc File src/kudu/master/master-test.cc: http://gerrit.cloudera.org:8080/#/c/18712/8/src/kudu/master/master-test.cc@1453 PS8, Line 1453: { > Could you please re-base this patch on the top of the master branch in the Done http://gerrit.cloudera.org:8080/#/c/18712/8/src/kudu/master/master-test.cc@1502 PS8, Line 1502: HASH > I'm not sure I understand why 'k' is needed when there is 'j' already? We reset j but not k. We need k to get the index range [0-3]. Using j alone or combined with i, I couldn't figure out a logic to get the index in each iteration. i j index 0 0 0 0 1 1 1 0 2 1 1 3 I could theoretically do index=i+i+j to make it work but doesn't look correct logically. http://gerrit.cloudera.org:8080/#/c/18712/8/src/kudu/master/master-test.cc@1529 PS8, Line 1529: ColumnSchemaPB* > style nit here and below: stick the asterisk to the type Done http://gerrit.cloudera.org:8080/#/c/18712/8/src/kudu/master/master-test.cc@1540 PS8, Line 1540: KuduPartialRow upper(&kTabl > style nit here and below: stick the asterisk to the type Done http://gerrit.cloudera.org:8080/#/c/18712/8/src/kudu/master/master-test.cc@1552 PS8, Line 1552: > style nit here and below: stick the ampersand to the type, not to the varia Done -- To view, visit http://gerrit.cloudera.org:8080/18712 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic8b8d90f70c39f13b838e858c870e08dacbdfcd3 Gerrit-Change-Number: 18712 Gerrit-PatchSet: 9 Gerrit-Owner: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy <[email protected]> Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Thu, 14 Jul 2022 19:17:28 +0000 Gerrit-HasComments: Yes
