Todd Lipcon has posted comments on this change. Change subject: KUDU-1775 (part 3): enforce max cell size and max PK size ......................................................................
Patch Set 3: (10 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 Done PS3, Line 459: 300 columns > I find '300 columns' to be a little strange when the Number of Columns reco Done 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? It was more convenient to write coverage for the "too-large-PK' case here in client-test rather than writing it in tablet_server-test, since I needed a schema with a composite key with multiple strings. All the utility code in tablet_server-test uses integer keys. Agreed that it's a little goofy that this is really focusing on a tablet-server bit of code, so doesn't belong here from a naming perspective, but same is true of a bunch of other tests like TestMutateDeletedRow, etc. 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 Done 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 cel Done 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 yea, that's what I meant here -- we could validate that the key is "legal", but it's probably best not to. Will rephrase. Line 480: Status s = ValidateInsertOrUpsertUnlocked(*op); > Would be nice to track invalid ops in a metric, though not required for thi added a TODO http://gerrit.cloudera.org:8080/#/c/5475/3/src/kudu/tablet/tablet_bootstrap.cc File src/kudu/tablet/tablet_bootstrap.cc: Line 1263: // TODO: get rid of redundant params below - they can be gotten from the Request > warning: missing username/bug in TODO [google-readability-todo] Done PS3, Line 1267: we > "after we" ? Done 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? Done -- 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-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
