[kudu-CR] KUDU-3371 [util] add NextOf method for ObjectIdGenerator

["Yingchun Lai (Code Review)" <ge...@cloudera.org>]

Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/19258 )

Change subject: KUDU-3371 [util] add NextOf method for ObjectIdGenerator
......................................................................


Patch Set 2:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/19258/2//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/19258/2//COMMIT_MSG@9
PS2, Line 9: use
> nit: using
Done


http://gerrit.cloudera.org:8080/#/c/19258/2//COMMIT_MSG@10
PS2, Line 10: usage cases
> nit: use cases
Done


http://gerrit.cloudera.org:8080/#/c/19258/2//COMMIT_MSG@12
PS2, Line 12: plus 1, to act as the upper exclusion key when scan.
> ... plus 1: that's to provide an exclusive upper bound when scanning
Done


http://gerrit.cloudera.org:8080/#/c/19258/2/src/kudu/util/oid_generator.h
File src/kudu/util/oid_generator.h:

http://gerrit.cloudera.org:8080/#/c/19258/2/src/kudu/util/oid_generator.h@47
PS2, Line 47: static size_t IdLength() { return 32; }
> Add 'constexpr' to the signature to allow for compile-time optimization?
I want to take advantage of the "Prefix Seek"[1] feature of rocksdb, for higher performance when scanning keys with the same prefix. The prefix is the container's id, I will use both IdLength() and NextOf() in next patches.

1. https://github.com/facebook/rocksdb/wiki/Prefix-Seek


http://gerrit.cloudera.org:8080/#/c/19258/2/src/kudu/util/oid_generator.cc
File src/kudu/util/oid_generator.cc:

http://gerrit.cloudera.org:8080/#/c/19258/2/src/kudu/util/oid_generator.cc@68
PS2, Line 68:   string next(id);
            :   next[next.size() - 1] += 1;
            :   return next;
> Just curious why to choose this approach instead of appending an extra char
The key in rocksdb is designed to combine contianer's id and block's id, and joined by a dot ("."), for example:
abcd.123
abcd.124
abce.123
abce.124

As mentioned above, I want to take advantage of the "Prefix Seek", when the upper is the successor key of scanned prefix, it can be taken.
Of course, any appending extra character can be taken advantage of it too, but '.' is a part of the real key, so it cannot be used since Scan["abcd", "abcd.") result empty.
I checked the ASCII order, '.' is 46, '0' is 48, 'A' is 65, 'a' is 97, any character which is greater than '.' can be used too. But if we change the combined key format in rocksdb, for example remove or change the join character '.', the upper key must be changed too, if we forget to do that, there may be issues(some key can not be scanned out, or etc).

So I choose this approach to make it to be definite correct.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I293baa379e8d64adc4ff93acdb0713aae2731c7d
Gerrit-Change-Number: 19258
Gerrit-PatchSet: 2
Gerrit-Owner: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yifan Zhang <ch...@163.com>
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Reviewer: Yuqi Du <sh...@gmail.com>
Gerrit-Comment-Date: Sun, 04 Dec 2022 12:39:17 +0000
Gerrit-HasComments: Yes