Rewrite leveldb_key to use a Reader/Writer class #960

wilhuff · 2018-03-21T21:17:56Z

No description provided.

wilhuff · 2018-03-21T21:18:56Z

Not sure this is a great idea, but it goes some ways toward alleviating some of the concerns you had in the prior PR and is relatively cheap to do.

var-const

Thanks for doing this.

In my view, this is cleaner, so unless you have concerns that the more rigid code structure can make some future refactoring harder, I'm for merging it.

The biggest improvement I see is no longer having to pass leveldb::Slice all over the place; I think it's nice having it encapsulated in a class. Not having to create uninitialized variables and pass them by pointer also reads much better. "Read" and "write" functions are now better grouped. One potential refactoring is using optional or StatusOr for Read functions, but I don't see a problem integrating that into Reader - ok would go away, but Reader would still have a reason for existence, managing the string slice.

var-const · 2018-03-21T21:46:41Z

Firestore/core/src/firebase/firestore/local/leveldb_key.cc

-  if (OrderedCode::ReadString(&tmp, result)) {
-    *src = MakeSlice(tmp);
-    return true;
+  bool ok() {


Nit: this function could be const.

var-const · 2018-03-21T21:46:46Z

Firestore/core/src/firebase/firestore/local/leveldb_key.cc

-void WriteComponentLabel(std::string *dest, ComponentLabel label) {
-  OrderedCode::WriteSignedNumIncreasing(dest, label);
-}
+  bool empty() {


Nit: this function could be const.

var-const · 2018-03-21T21:46:53Z

Firestore/core/src/firebase/firestore/local/leveldb_key.cc

-      *label = static_cast<ComponentLabel>(raw_result);
-      return true;
-    }
+  leveldb::Slice source() {


Nit: this function could be const.

var-const · 2018-03-21T21:47:22Z

Firestore/core/src/firebase/firestore/local/leveldb_key.cc

-}
+class Writer {
+ public:
+  std::string result() {


Nit: this function could be const.

var-const · 2018-03-21T21:55:24Z

Firestore/core/src/firebase/firestore/local/leveldb_key.cc

+  }
+
+  /** OrderedCode::ReadSignedNumIncreasing adapted to leveldb::Slice. */
+  int64_t ReadSignedNumIncreasing() {


Do you think some of these lower-level functions could now be made private? I think it's also a small improvement that using a class can bring: previously, there also was a separation between these and domain-level functions (e.g., ReadBatchId), but it was only in the comments. Candidates for private are:

ReadSignedNumIncreasing;

ReadString;

ReadInt32;

(maybe) ReadLabeledInt32;

(maybe) WriteLabeledInt32.

I think nearly all the lowest-level ones could be made private if Describe were a member on the Reader. I'll attempt this next.

wilhuff

Sorry -- I should have made it clearer that this was essentially a proof-of-concept rather than any kind of done state. Comments need to be updated, constness applied, etc. I'll reassign to you once I've gotten this cleaned up.

wilhuff · 2018-03-21T22:09:13Z

Firestore/core/src/firebase/firestore/local/leveldb_key.cc

+  }
+
+  /** OrderedCode::ReadSignedNumIncreasing adapted to leveldb::Slice. */
+  int64_t ReadSignedNumIncreasing() {


I think nearly all the lowest-level ones could be made private if Describe were a member on the Reader. I'll attempt this next.

googlebot · 2018-03-22T01:44:09Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

wilhuff · 2018-03-22T01:48:53Z

Comments updated, const applied, low-level details made private, and a few more bits of refactoring to make this all work. PTAL.

var-const

LGTM, just a couple of typos.

var-const · 2018-03-22T17:44:57Z

Firestore/core/src/firebase/firestore/local/leveldb_key.cc

+   * Reads component labels and strings from the key until it finds a component
+   * label other that ComponentLabel::PathSegment (or the key is exhausted).
+   * All matched path segments are assembled into a ResourcePath and wrapped in
+   * an DocumentKey.


Nit: I think it should be "a DocumentKey".

Right. Sorry.

var-const · 2018-03-22T17:45:58Z

Firestore/core/src/firebase/firestore/local/leveldb_key.cc

+   * Otherwise returns whether or not the component label is equal to the
+   * `expected_label` and advances the Rader to the next unread byte.
+   */
+  ABSL_MUST_USE_RESULT


This looks interesting.

I missed one in my early implementation and this caused me to go find this.

var-const · 2018-03-22T17:46:12Z

Firestore/core/src/firebase/firestore/local/leveldb_key.cc

+   * If the read is unsuccessful, returns false, and fails the Reader.
+   *
+   * Otherwise returns whether or not the component label is equal to the
+   * `expected_label` and advances the Rader to the next unread byte.


Nit: s/Rader/Reader.

Thank you for the careful review!

var-const · 2018-03-22T17:48:32Z

Firestore/core/src/firebase/firestore/local/leveldb_key.cc

+  }
+
+  /**
+   * For each segment in the given resource path writes an


Nit: I think it should be "a ComponentLabel::PathSegment", not "an" (might be wrong).

You're right. This comes from the fact that the previous enumeration value here was "FSTComponentLabelPathSegment". In that case "an" worked because we spell the prefix out. Now that the prefix is gone it should be "a".

wilhuff added the api: firestore label Mar 21, 2018

wilhuff assigned var-const Mar 21, 2018

wilhuff requested a review from var-const March 21, 2018 21:17

googlebot added the cla: yes label Mar 21, 2018

var-const approved these changes Mar 21, 2018

View reviewed changes

wilhuff commented Mar 21, 2018

View reviewed changes

wilhuff assigned wilhuff and unassigned var-const Mar 21, 2018

wilhuff force-pushed the wilhuff/leveldb-key branch from 3b40fa7 to e5cffe2 Compare March 22, 2018 01:00

wilhuff force-pushed the wilhuff/leveldb-key-experiment branch 2 times, most recently from b95b468 to 467079b Compare March 22, 2018 01:44

googlebot added cla: no and removed cla: yes labels Mar 22, 2018

wilhuff changed the base branch from wilhuff/leveldb-key to master March 22, 2018 01:45

wilhuff added cla: yes and removed cla: no labels Mar 22, 2018

Rewrite leveldb_key to use a Reader/Writer class

8fd20be

wilhuff force-pushed the wilhuff/leveldb-key-experiment branch from 467079b to 8fd20be Compare March 22, 2018 01:47

wilhuff assigned var-const and unassigned wilhuff Mar 22, 2018

var-const approved these changes Mar 22, 2018

View reviewed changes

Review feedback

12ad636

wilhuff merged commit 4c3fb51 into master Mar 24, 2018

wilhuff deleted the wilhuff/leveldb-key-experiment branch March 24, 2018 17:23

minafarid pushed a commit to minafarid/firebase-ios-sdk that referenced this pull request Jun 6, 2018

Rewrite leveldb_key to use a Reader/Writer class (firebase#960)

0981b0c

firebase locked and limited conversation to collaborators Nov 6, 2019

Rewrite leveldb_key to use a Reader/Writer class #960

Rewrite leveldb_key to use a Reader/Writer class #960

Uh oh!

Conversation

wilhuff commented Mar 21, 2018

Uh oh!

wilhuff commented Mar 21, 2018

Uh oh!

var-const left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wilhuff left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

googlebot commented Mar 22, 2018

Uh oh!

wilhuff commented Mar 22, 2018

Uh oh!

var-const left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!