Todd Lipcon has posted comments on this change.

Change subject: improvements to /table
......................................................................


Patch Set 1:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/4021/1//COMMIT_MSG
Commit Message:

Line 29: 5. The ALL_CAPS enum names for the encoding and compression types
hrm, I actually prefer the ALL_CAPS, since anyone using the API (or SQL) would 
have to use the actual enum name, and now you're making them convert back, no?


http://gerrit.cloudera.org:8080/#/c/4021/1/src/kudu/master/master-path-handlers.cc
File src/kudu/master/master-path-handlers.cc:

Line 126:   if (a.second == RaftPeerPB::LEADER) {
hrm, I have a hard time verifying in my head whether this maintains a proper 
total order (it probably does, but I've made mistakes in this sort of function 
before, and had it cause a crash on some STL implementations which require 
"consistent" comparators).

how about something like:

int RoleToSortIndex(RaftPeerPB::Role r) {
  switch (r) {
    case LEADER: return 0;
    default: return 1 + static_cast<int>(r);
  }
}

and then compare RoleToSortIndex(a) < RoleToSortIndex(b)?


Line 259:   *output << "<h3>Partition schema & range bounds</h3>";
&amp;


Line 265:                           
partition_schema.RangePartitionDebugString(pair.first,
need to HTML Escape this (otherwise someone could make a range bound with a 
<script> tag)


http://gerrit.cloudera.org:8080/#/c/4021/1/src/kudu/server/webui_util.cc
File src/kudu/server/webui_util.cc:

PS1, Line 136:   *output << "  'kudu.table_name' = '" << table_name << "',\n";
             :   *output << "  'kudu.master_addresses' = '" << master_addresses 
<< "',\n";
             :   *output << "  'kudu.key_columns' = '" << 
JoinElements(key_columns, ", ") << "'\n";
now that I look at this code again, I think these need HTML escapes


-- 
To view, visit http://gerrit.cloudera.org:8080/4021
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib6724348b1cd199c4d651c1282f1eadb58226bea
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Will Berkeley <wdberke...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <t...@apache.org>
Gerrit-HasComments: Yes

Reply via email to