Adar Dembo has posted comments on this change. Change subject: KUDU-1775 (part 3): enforce max cell size and max PK size ......................................................................
Patch Set 3: (9 comments) http://gerrit.cloudera.org:8080/#/c/5475/3/docs/schema_design.adoc File docs/schema_design.adoc: PS3, Line 455: Nit: extra space here PS3, Line 459: 300 columns I find '300 columns' to be a little strange when the Number of Columns recommendation above mentions '200 columns'. Is this discrepancy intentional? If so, may want to clarify a bit. http://gerrit.cloudera.org:8080/#/c/5475/3/src/kudu/client/client-test.cc File src/kudu/client/client-test.cc: Line 3909: // Test trying to insert a row with an encoded key that is too large. Perhaps add an equivalent for the Java client? Not really sure why this is worth testing in the clients, to be honest; it's entirely a server-side change. http://gerrit.cloudera.org:8080/#/c/5475/3/src/kudu/integration-tests/ts_recovery-itest.cc File src/kudu/integration-tests/ts_recovery-itest.cc: PS3, Line 232: configuratio configuration http://gerrit.cloudera.org:8080/#/c/5475/3/src/kudu/tablet/tablet.cc File src/kudu/tablet/tablet.cc: Line 414: // TODO(todd): test what happens if we restart with an invalid op in our log! Doesn't your new test in ts_recovery-itest do this, by reducing the max cell size and restarting? PS3, Line 452: // Don't do any validation on delete. This is important to allow users to : // delete a row if a row with a too-large key was inserted on a previous version : // that had no limits. Is there really any validation to be done on delete? I guess we can look at the key size, but that's about it, right? Line 480: Status s = ValidateInsertOrUpsertUnlocked(*op); Would be nice to track invalid ops in a metric, though not required for this patch. http://gerrit.cloudera.org:8080/#/c/5475/3/src/kudu/tablet/tablet_bootstrap.cc File src/kudu/tablet/tablet_bootstrap.cc: PS3, Line 1267: we "after we" ? http://gerrit.cloudera.org:8080/#/c/5475/3/src/kudu/tserver/tablet_server-test.cc File src/kudu/tserver/tablet_server-test.cc: Line 328: // This should generate one error into per_row_errors. But it generates two errors; maybe this needs to be updated? -- To view, visit http://gerrit.cloudera.org:8080/5475 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ib6b94cffd9c3efbe80a4b31e9272b376a4e41d15 Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: Yes
