[WIP] Rocks condition with update #22

stevelorddremio · 2021-03-01T19:51:50Z

This is the RocksDB implementation for update().
Ready for review are implementations for:
L1,
L2,
L3,
Value,
CommitMetadata
Fragment
Ref - Tag
Ref - Branch (missing a new tests on KeyMutations)

versioned/tiered/rocksdb/src/main/java/org/projectnessie/versioned/rocksdb/RocksBaseValue.java

stevelorddremio · 2021-03-02T23:38:44Z

versioned/tiered/rocksdb/src/main/java/org/projectnessie/versioned/rocksdb/RocksBaseValue.java

+    switch (updateFunction.getOperator()) {
+      case REMOVE:
+        if (childPath == null || !fieldIsList(segment, childPath)) {
+          throw new UnsupportedOperationException();


A comment you will probably get anyway is to provide more information in the exception about what is failing.

versioned/tiered/rocksdb/src/main/java/org/projectnessie/versioned/rocksdb/RocksBaseValue.java

stevelorddremio · 2021-03-02T23:49:36Z

versioned/tiered/rocksdb/src/main/java/org/projectnessie/versioned/rocksdb/RocksL1.java

+        );
+        break;
+      case KEY_MUTATIONS_KEY:
+        if (!childPath.getChild().get().getChild().isPresent()) {


Just think we may want some utility methods as there is a lot of commonality in drilling through the PathSegment hierarchy.

The Optionals and path structure do make this a bit ugly. I don't have any good ideas for cleaning this up though. Do you have any suggestions?

...tiered/rocksdb/src/test/java/org/projectnessie/versioned/rocksdb/TestUpdateFunctionBase.java

...red/rocksdb/src/test/java/org/projectnessie/versioned/rocksdb/TestUpdateFunctionRocksL1.java

stevelorddremio · 2021-03-03T18:32:33Z

versioned/tiered/rocksdb/src/main/java/org/projectnessie/versioned/rocksdb/RocksBaseValue.java

+              set(segment, childPath, setFunction.getValue());
+            } else if (fieldIsList(segment, childPath) != (setFunction.getValue().getType() == Entity.EntityType.LIST)) {
+              throw new UnsupportedOperationException();
+            } else if (ID.equals(segment)) {


Should we handle set equals for dt field here also from BaseValue?

I added it now, but I doubt it would actually be used.

They did actually use this in their GC prototyping. Subsequently they dropped it, but shows they sometimes have a need.

OK. It's implemented now, so we should be safe if they start updating date time.

I may have missed it, but can you add some tests too?

This is tested in the test classes for the implementations, such as TestUpdateFunctionRocksL1.

...red/rocksdb/src/test/java/org/projectnessie/versioned/rocksdb/TestUpdateFunctionRocksL1.java

stevelorddremio · 2021-03-03T18:52:52Z

...red/rocksdb/src/test/java/org/projectnessie/versioned/rocksdb/TestUpdateFunctionRocksL1.java

+            .builder(RocksL1.KEY_MUTATIONS)
+            .position(0)
+            .name(RocksL1.KEY_MUTATIONS_MUTATION_TYPE)
+            .build(), Entity.ofNumber(1)));


Avoid magic numbers, particularly as it is input and output data.

stevelorddremio · 2021-03-03T18:56:34Z

...red/rocksdb/src/test/java/org/projectnessie/versioned/rocksdb/TestUpdateFunctionRocksL2.java

+
+  @Test
+  void childrenSetEquals() {
+    final UpdateExpression updateExpression =


This is very similar to the childrenSetEquals() test for RocksL1. Could we have a generic test?

I see what you are thinking here, but it isn't quite as simple as it looks. RocksL1 and RocksL2 do not get the children from a common base class.
Another idea here is to parameterize the Update test classes, move the list tests into the base class and then add parameters for accessing the actual lists. This would get a little ugly with the parameters but would ensure that all lists are tested the same way.
What do you think?

Yes, I'm leaning that way to parameterize the tests. I've been moving that way in TestUpdateFunctionRocksRef for branch in rocks-update-branch. Although I haven't moved anything into the base class as it is Branch specific.

...ed/rocksdb/src/test/java/org/projectnessie/versioned/rocksdb/TestUpdateFunctionRocksRef.java

versioned/tiered/rocksdb/src/main/java/org/projectnessie/versioned/rocksdb/RocksBaseValue.java

stevelorddremio · 2021-03-03T23:49:14Z

versioned/tiered/rocksdb/src/main/java/org/projectnessie/versioned/rocksdb/RocksBaseValue.java

+              set(segment, childPath, setFunction.getValue());
+            } else if (fieldIsList(segment, childPath) != (setFunction.getValue().getType() == Entity.EntityType.LIST)) {
+              throw new UnsupportedOperationException();
+            } else if (ID.equals(segment)) {


I may have missed it, but can you add some tests too?

stevelorddremio · 2021-03-03T23:54:37Z

...red/rocksdb/src/test/java/org/projectnessie/versioned/rocksdb/TestUpdateFunctionRocksL2.java

+
+  @Test
+  void childrenSetEquals() {
+    final UpdateExpression updateExpression =


Yes, I'm leaning that way to parameterize the tests. I've been moving that way in TestUpdateFunctionRocksRef for branch in rocks-update-branch. Although I haven't moved anything into the base class as it is Branch specific.

…ition. Thrown exception did not match API description.

…n and not UpdateExpression

…sing.

kylep-dremio · 2021-03-09T20:18:20Z

versioned/tiered/rocksdb/src/main/java/org/projectnessie/versioned/rocksdb/EntityConverter.java

+
+import org.projectnessie.versioned.store.Entity;
+
+public class EntityConverter {


Why public? JavaDocs?

No reason for public. Added JavaDocs.

kylep-dremio · 2021-03-09T20:18:25Z

versioned/tiered/rocksdb/src/main/java/org/projectnessie/versioned/rocksdb/EntityConverter.java

+   * @param entity the entity to convert
+   * @return the object converted from the entity
+   */
+  public static ValueProtos.Commit entityToCommit(Entity entity) {


Why public?

Changed to package.

kylep-dremio · 2021-03-09T20:18:56Z

versioned/tiered/rocksdb/src/main/java/org/projectnessie/versioned/rocksdb/EntityConverter.java

+    if (!entity.getType().equals(Entity.EntityType.MAP)) {
+      throw new UnsupportedOperationException();
+    }
+    ValueProtos.Commit.Builder builder = ValueProtos.Commit.newBuilder();


Style should be that we append final to everything, unless you have a reason to do so otherwise.

kylep-dremio · 2021-03-09T20:19:30Z

versioned/tiered/rocksdb/src/main/java/org/projectnessie/versioned/rocksdb/EntityConverter.java

+          if (!level0.getValue().getType().equals(Entity.EntityType.LIST)) {
+            throw new UnsupportedOperationException();
+          }
+          for (Entity level1 : level0.getValue().getList()) {


Won't this throw if it's not a list already?

Etc. for all other checks of this sort.

You are correct. Removed the checks.

kylep-dremio · 2021-03-09T20:19:40Z

versioned/tiered/rocksdb/src/main/java/org/projectnessie/versioned/rocksdb/EntityConverter.java

+    }
+    ValueProtos.Commit.Builder builder = ValueProtos.Commit.newBuilder();
+
+    for (Map.Entry<String, Entity> level0 : entity.getMap().entrySet()) {


Won't this throw already if it's not a map?

kylep-dremio · 2021-03-09T20:32:18Z

versioned/tiered/rocksdb/src/main/java/org/projectnessie/versioned/rocksdb/RocksBaseValue.java

+                appendToList(segment, childPath, Collections.singletonList(setFunction.getValue()));
+              }
+            } else {
+              throw new UnsupportedOperationException(String.format("The append to list operation is not valid for \"%s\"", segment));


I'd prefer stylistically to not have the throws in the else, but to have them in the if. This way you don't even need the else, remove nesting, and don't need to refer to above in the code to find out why the exception is triggered.

Updated what I could to remove some of the nesting and to move the throws up higher.

kylep-dremio · 2021-03-09T20:33:57Z

versioned/tiered/rocksdb/src/main/java/org/projectnessie/versioned/rocksdb/RocksFragment.java

 */
 class RocksFragment extends RocksBaseValue<Fragment> implements Fragment {
  static final String KEY_LIST = "keys";

-  private final ValueProtos.Fragment.Builder builder = ValueProtos.Fragment.newBuilder();
+  private final ValueProtos.Fragment.Builder fragmentBuilder = ValueProtos.Fragment.newBuilder();


Why the rename?

In my opinion, builder is an overly generic name. This gets problematic if we have a scope that also has another type of Builder, such as a Key.Builder.
Is there a reason to keep the generic name?

kylep-dremio · 2021-03-09T20:35:01Z

versioned/tiered/rocksdb/src/main/java/org/projectnessie/versioned/rocksdb/RocksFragment.java

+          .newBuilder()
+          .addAllElements(v.getList().stream().map(Entity::getString).collect(Collectors.toList()))
+          .build()
+      ).collect(Collectors.toList()));


You should be able to avoid intermediate list creation by adding incrementally and not using addAllKeys.

Please apply this to all applicable locations.

kylep-dremio · 2021-03-09T20:35:22Z

versioned/tiered/rocksdb/src/main/java/org/projectnessie/versioned/rocksdb/RocksFragment.java

+
+  @Override
+  protected void appendToList(String fieldName, ExpressionPath.PathSegment childPath, List<Entity> valuesToAdd) {
+    if (new PathPattern().matches(childPath)) {


This looks odd, why isn't matches a static?

I think the PathPattern class needs to be rethought a bit, it looks like it always creates a new one.

I can see that you would like to remove the PathPattern class as you don't see any benefit to it.
If I remove it, then I will be dealing with ExpressionPath.PathSegment and I will receive a lot of feedback about how difficult it is to understand what is going on.

new PathPattern().nameEquals("foo").anyPosition()
is equivalent to:
if path != null && path.isName() && "foo".equals(path.asName().getName()) && path.getChild().isPresent() && path.getChild().get().isPosition())

It's not that I want to have it removed, just that it looks odd to have to new it every time it's used. I wonder if it wouldn't be better to have a static builder or something?

This could be improved, and we could have the path patterns stored static in the classes. I'll leave as is for now, since you likely have a different approach in mind.

The PathPattern has been updated. It is still created new each time it is used. The next step would be to extract the PathPatterns into static variables.

kylep-dremio · 2021-03-09T20:36:43Z

versioned/tiered/rocksdb/src/main/java/org/projectnessie/versioned/rocksdb/RocksL2.java

+  @Override
+  protected void remove(String fieldName, ExpressionPath.PathSegment path) {
+    if (TREE.equals(fieldName)) {
+      List<ByteString> updatedChildren = new ArrayList<>(l2Builder.getTreeList());


…for PathPattern.

… implements the Visitor pattern.

…o rocks-update-branch

stevelorddremio commented Mar 3, 2021

View reviewed changes

...red/rocksdb/src/test/java/org/projectnessie/versioned/rocksdb/TestUpdateFunctionRocksL1.java Outdated Show resolved Hide resolved

stevelorddremio commented Mar 3, 2021

View reviewed changes

stevelorddremio commented Mar 4, 2021

View reviewed changes

stevelorddremio and others added 26 commits March 5, 2021 09:17

rebase on main

82faa37

Rocks checkstyle

e241645

Rocks PR updates. Correct AbstractTestStore put test for failing cond…

451dc1d

…ition. Thrown exception did not match API description.

Rocks PR updates. Revert mongodb changes.

bf5ba27

RocksDB Ext PR updates

2e2ca1c

RocksDB ext PR updates

6760853

Rocks ext PR updates

830a5b6

Rocks update - initial skeleton

a85e2e3

Rocks update add update visitor and start implementing update on L1

eb77566

Rocks update

3c59809

Rocks UpdateExpression SetClause test

e4da222

Rocks Update L1 Id Set and Remove tests

96d9ff4

Rocks update() consolidate tests

d66113c

Rocks Update() consolidate tests

a39be60

RocksDB update() streams initial handling.

70d94ed

RocksDB Revert Function back to be used purely for ConditionExpressio…

b62cb30

…n and not UpdateExpression

Rocks rebase

6369e55

Rocks update for L1 children, ancestors and complete key list

93e10de

Fixed up some compile issues.

b2a50ea

Refactored and further implemented update for RocksL1.

991541f

Refactored the update unit tests.

fe1ca73

Implemented update for RocksL2.

195cb03

Implemented update for RocksL3.

95c3b78

Partially mplemented update for RocksFragment.

2235baf

Partially implemented update for RocksRef. Some code cleanup.

f687c6c

Implemented update for RocksValue and RocksWrappedValue.

b50ee84

Norman Jordan added 2 commits March 9, 2021 09:44

Refactored some code to make it more readable for updates.

6a86fd6

Updated to get the AbstractITTieredVersionStore tests running and pas…

33d20e3

…sing.

kylep-dremio requested changes Mar 9, 2021

View reviewed changes

Norman Jordan and others added 27 commits March 9, 2021 14:17

Implemented more of remove for RocksRef. Added some Javadoc comments …

dc8b996

…for PathPattern.

Updated EntityConverter based on PR feedback.

f6de458

Removed some checks that were reduntant.

38af3a5

Fixed a typo.

d4adde7

Updated PathPattern based on PR feedback.

59df7bb

Updated RocksBaseValue based on PR feedback.

1cdeb7c

Removed stream collection to a list where possible.

1e698b7

Rocks commits appendToList added

eab0e95

Rocks add commits tests

d0b5d4e

Rocks add set equals for commits

1fdaae4

Rocks update() for branch.commits.delta

c9c27d2

Rocks branch.commits.delta and key mutations appendToList added

f9c4217

Refactored to make more extensive use of PathPattern. PathPattern now…

7289cc0

… implements the Visitor pattern.

Updated the PathPattern to support prefix matching.

2123d6c

Removed the fieldIsList() method.

ccb9c42

Added final to a local variable.

fa385b6

Updated some Javadoc comments.

2ec82e7

Rocks add branch.commits.keys set-equals handling

a947cc2

Rocks checkstyle

f0b903a

Merge branch 'rocks-condition-with-update' into rocks-update-branch

b5d5e76

Rocks refactor branch.commits set equals into new structure.

409ff56

Moved the PathPattern creation to static variables.

ff7e849

Rocks branch.commits.keys.key appendToList impl

4714642

Merge remote-tracking branch 'origin/rocks-condition-with-update' int…

f2b3b1e

…o rocks-update-branch

Rocks checkstyle

27d5731

Rocks internal PR comments.

e2541b0

Rocks Enable disabled tests

25817ca


		import org.projectnessie.versioned.store.Entity;

		public class EntityConverter {

[WIP] Rocks condition with update #22

Are you sure you want to change the base?

[WIP] Rocks condition with update #22

Conversation

stevelorddremio commented Mar 1, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

normanj-bitquill Mar 10, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevelorddremio commented Mar 1, 2021 •

edited

Loading

normanj-bitquill Mar 10, 2021 •

edited

Loading