Introduce operation attributes #2333

brfrn169 · 2024-11-13T04:45:50Z

Description

This PR introduces operation attributes, providing the capability to include additional key-value information in operations.

Related issues and/or PRs

N/A

Changes made

Added operation attributes to Operation.
Updated operation builders to support operation attributes.

Checklist

I have commented my code, particularly in hard-to-understand areas.
I have updated the documentation to reflect the changes.
Any remaining open issues linked to this PR are documented and up-to-date (Jira, GitHub, etc.).
Tests (unit, integration, etc.) have been added for the changes.
My changes generate no new warnings.
Any dependent changes in other PRs have been merged and published.

Additional notes (optional)

N/A

Release notes

Introduced operation attributes, providing the capability to include additional key-value information in operations.

brfrn169 · 2024-11-13T04:46:12Z

core/src/main/java/com/scalar/db/api/Operation.java

@@ -25,18 +25,21 @@ public abstract class Operation {
  @Nullable private String namespace;
  private String tableName;
  private Consistency consistency;
+  private final ImmutableMap<String, String> attributes;


Added operation attributes to Operation.

komamitsu · 2024-11-14T01:56:14Z

core/src/main/java/com/scalar/db/api/DeleteBuilder.java

    @Nullable Key clusteringKey;
    @Nullable com.scalar.db.api.Consistency consistency;
    @Nullable MutationCondition condition;
+    Map<String, String> attributes = new HashMap<>();


This can be final?

Fixed in 241b26e. Thanks.

komamitsu · 2024-11-14T02:06:56Z

core/src/main/java/com/scalar/db/api/Operation.java

+   *
+   * @return the attributes
+   */
+  public Map<String, String> getAttributes() {


[minor] I think this can return ImmutableMap<> to make it easy to construct Immutable objects without copying on the caller side.

Since getAttributes() is a user-facing API, I didn’t expose the implementation of the Map interface here. I usually use immutable collections like ImmutableMap for internal APIs, but I avoid using them in exposed APIs. What do you think?

Since getAttributes() is a user-facing API

Oh, I see. Agreed. Let's go with returning Map as is.

komamitsu · 2024-11-14T02:11:49Z

core/src/main/java/com/scalar/db/api/UpdateBuilder.java

    final Map<String, Column<?>> columns = new LinkedHashMap<>();
    @Nullable Key clusteringKey;
    @Nullable MutationCondition condition;
+    Map<String, String> attributes = new HashMap<>();


Same as above.

Fixed in 241b26e. Thanks.

feeblefakie

I left several very minor comments on the definition order consistency.
But, it's my preference, so it's up to you.
LGTM! Thank you!

feeblefakie · 2024-11-14T03:52:06Z

core/src/main/java/com/scalar/db/api/Delete.java

      @Nullable MutationCondition condition) {
-    super(namespace, tableName, partitionKey, clusteringKey, consistency, condition);
+    super(namespace, tableName, partitionKey, clusteringKey, consistency, attributes, condition);


Just wondering if there is any reason for putting it in the middle of existing arguments?

The reason is that the first six arguments are for Operation. I arranged the arguments in hierarchical order: Operation, Mutation, and Delete.

feeblefakie · 2024-11-14T03:53:38Z

core/src/main/java/com/scalar/db/api/Delete.java

        .add("condition", getCondition())
+        .add("consistency", getConsistency())


It is a super minor thing, but I think it's slightly better that the order is consistent with the order of the constructor arguments.

Let me make them consistent. Thanks.

Fixed in c00f0b4. Thanks.

feeblefakie · 2024-11-14T04:13:33Z

core/src/main/java/com/scalar/db/api/DeleteBuilder.java

@@ -90,10 +99,31 @@ public Buildable consistency(com.scalar.db.api.Consistency consistency) {
      return this;
    }

+    @Override
+    public Buildable attribute(String name, String value) {


It's again a super minor comment, but the definition order of the variables (consistency, condition, and attributes) and the method definition order can be consistent to make it slightly better.
It's most probably my preference, so I don't argue about it.

Let me make them consistent. Thanks.

Ah, I thought I would do that this time, but it looks like it’s very time-consuming… Actually, the original method definition order is already inconsistent. Let’s tackle it when we have more time. 🙇

Torch3333

LGTM, thank you!

komamitsu

LGTM! 👍

Introduce operation attributes

7ea2f57

brfrn169 added the enhancement New feature or request label Nov 13, 2024

brfrn169 self-assigned this Nov 13, 2024

brfrn169 commented Nov 13, 2024

View reviewed changes

brfrn169 requested review from komamitsu, feeblefakie and Torch3333 November 13, 2024 05:15

komamitsu reviewed Nov 14, 2024

View reviewed changes

feeblefakie approved these changes Nov 14, 2024

View reviewed changes

brfrn169 added 2 commits November 14, 2024 14:37

Fix based on feedback

241b26e

Fix based on feedback

c00f0b4

brfrn169 requested a review from komamitsu November 14, 2024 06:03

Torch3333 approved these changes Nov 14, 2024

View reviewed changes

Merge branch 'master' into introduce-operation-attributes

106b19e

komamitsu approved these changes Nov 14, 2024

View reviewed changes

brfrn169 merged commit 7140bee into master Nov 14, 2024
48 checks passed

brfrn169 deleted the introduce-operation-attributes branch November 14, 2024 09:50

feeblefakie pushed a commit that referenced this pull request Nov 14, 2024

Introduce operation attributes (#2333)

4f5a10f

feeblefakie mentioned this pull request Nov 14, 2024

Backport to branch(3) : Introduce operation attributes #2339

Merged

brfrn169 mentioned this pull request Nov 18, 2024

Use operation attributes for implicit pre-read and insert mode of Put #2351

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce operation attributes #2333

Introduce operation attributes #2333

brfrn169 commented Nov 13, 2024 •

edited

Loading

brfrn169 Nov 13, 2024

komamitsu Nov 14, 2024

brfrn169 Nov 14, 2024

komamitsu Nov 14, 2024

brfrn169 Nov 14, 2024

komamitsu Nov 14, 2024

komamitsu Nov 14, 2024

brfrn169 Nov 14, 2024 •

edited

Loading

feeblefakie left a comment

feeblefakie Nov 14, 2024

brfrn169 Nov 14, 2024

feeblefakie Nov 14, 2024

brfrn169 Nov 14, 2024

brfrn169 Nov 14, 2024

feeblefakie Nov 14, 2024

brfrn169 Nov 14, 2024

brfrn169 Nov 14, 2024

Torch3333 left a comment

komamitsu left a comment

		.add("condition", getCondition())
		.add("consistency", getConsistency())

Introduce operation attributes #2333

Introduce operation attributes #2333

Conversation

brfrn169 commented Nov 13, 2024 • edited Loading

Description

Related issues and/or PRs

Changes made

Checklist

Additional notes (optional)

Release notes

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

brfrn169 Nov 14, 2024 • edited Loading

Choose a reason for hiding this comment

feeblefakie left a 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

Torch3333 left a comment

Choose a reason for hiding this comment

komamitsu left a comment

Choose a reason for hiding this comment

brfrn169 commented Nov 13, 2024 •

edited

Loading

brfrn169 Nov 14, 2024 •

edited

Loading