Alexey Serbin 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 3: (6 comments) A quick top-level review. http://gerrit.cloudera.org:8080/#/c/18712/3//COMMIT_MSG Commit Message: PS3: Overall looks good. There is some difference in terms: in master's webUI BUCKET is used for the partition/bucket index; in tablet server's webUI that's PARTITION. I'm not sure how to consolidate these differences. Maybe, for the master's webUI, instead of mixing both the schema and the bucket index information into the same column 'Hash Schema', there should be two separate columns: 'Hash Schema' and 'Hash Partition'? http://gerrit.cloudera.org:8080/#/c/18712/3/src/kudu/common/partition.h File src/kudu/common/partition.h: http://gerrit.cloudera.org:8080/#/c/18712/3/src/kudu/common/partition.h@409 PS3, Line 409: nit: misaligned indent http://gerrit.cloudera.org:8080/#/c/18712/3/src/kudu/common/partition.h@410 PS3, Line 410: style nit: stick the ampersand to the type http://gerrit.cloudera.org:8080/#/c/18712/3/src/kudu/common/partition.cc File src/kudu/common/partition.cc: http://gerrit.cloudera.org:8080/#/c/18712/3/src/kudu/common/partition.cc@1087 PS3, Line 1087: // Get the custom hash schema information, if present, for this partition It's possible to find hash schema for a range using the helper map hash_schema_idx_by_encoded_range_start_: you can see how to do so in PartitionSchema::GetHashSchemaForRange(). http://gerrit.cloudera.org:8080/#/c/18712/3/src/kudu/common/partition.cc@1096 PS3, Line 1096: lower Why to compare the upper_boundary with lower? http://gerrit.cloudera.org:8080/#/c/18712/3/src/kudu/master/master-test.cc File src/kudu/master/master-test.cc: http://gerrit.cloudera.org:8080/#/c/18712/3/src/kudu/master/master-test.cc@290 PS3, Line 290: hash_schema->set_seed(hash_dimension.seed); Could you separate this into its own changelist? I guess this change isn't related to the essence of this patch, right? -- 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: 3 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-Comment-Date: Wed, 13 Jul 2022 06:15:56 +0000 Gerrit-HasComments: Yes
