Dan Burkert has posted comments on this change.

Change subject: KUDU-1308 [c++-client]: support tables with non-covering range 

Patch Set 2:


File src/kudu/client/client-internal.cc:

Line 140:     vector<uint32_t> required_feature_flags) {
> This is never used?
oops, forgot the last plumbing step.

File src/kudu/client/client-test.cc:

Line 383: // Creates a table with 'num_replicas', split into tablets based on 
        :   // (or single tablet if 'split_rows' is empty).
> Update this comment.

Line 1045:   KuduPartialRow* a_lower_bound = schema_.NewRow();
> Nit: to be completely safe, we ought to store these in unique_ptrs, then re
We already leak the split rows in the same failure scenario, so I don't think 
it's worth doing.  Pretty unfortunate, as it's a strong indicator that our 
client API is too difficult to use correctly.  If I could do it over again, I 
would change

  KuduTableCreator& split_rows(const std::vector<const KuduPartialRow*>& 

  KuduTableCreator& add_split_row(KuduPartialRow* split_row);

Line 1140:   {
> Nit: one-line comments for these test cases too?

File src/kudu/client/client.h:

Line 368: Add a partition range bound to the table with an inclusive lower 
bound and
        :   // exclusive upper bound.
> I thought we were going to offer inclusive upper bounds? That's what I reme
We can, as another method.  I would prefer to leave that to a follow-up commit.

File src/kudu/client/meta_cache.cc:

Line 755:   if (tablets_by_key.empty()) {
> Should we do this before upper_bound(), since it doesn't depend on its resu
yes, but because I removed the duplicate Status constructors it ends up being 
easier to keep it.  If the collection is empty the upper_bound call is a no-op.

Line 756:     return Status::NotFound("No tablet covering the requested range 
> Nit: you're creating the same Status in three different places. Perhaps use

Line 758:     // The requested partition key is before all tablets
> Nit: end with period. Do the same for the others here, please.

Line 762:     *remote_tablet = itr->second;
> Is the dereference on the RHS safe? Does itr == begin() imply that there's 
It does in this case, since we already checked that the collection is non-empty.

Line 763: std::prev(itr)->second->partition().partition_key_end() > 
rpc.partition_key() ||
> Likewise, are these std::prev() calls safe? What if itr == end()? Is a prev
std::prev is safe in this case, since we know the collection is non-empty, and 
that itr is not pointing to the first element (::begin()).

Line 811:   ts_cache_.clear();
> This doesn't leak the map values?

File src/kudu/client/scanner-internal.cc:

Line 308: else if (!s.ok()) {
        :       return s;
        :     }
> Can do RETURN_NOT_OK(s) here instead.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1cb12704c5e9792ee6e5831568bc52b1a713f8d5
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <d...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <a...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <d...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <t...@apache.org>
Gerrit-HasComments: Yes

Reply via email to