Add 'fields' parameter to ProxySpace #263

ArtDu · 2022-09-15T18:34:02Z

Add fields option to crud proxy methods

Tests
Changelog
Documentation
Cleanup the code for review. See checklist

Closes #236

akudiyar

I liked it overall, but I believe that there's a better way. Please take a look at the comments below.

I think that Options should be much like the Conditions class, which is actually a builder and it is convenient to call a select method like space.select(Conditions.any()), for example. We should have something like that for Options. Of course, in this case, we'll have to tell the user if any actual options aren't supported by the particular client instance (in the same way as we do in Conditions). Serialization of Options instances will happen internally, no need to force users to use additional builder instances and call build(), that's ugly. But the difference will be that we can have different Options classes for each operation. So for select it will be SelectOptions, for insertMany it will be InsertManyOptions etc.

An example of how the Options class can be used:

    space.select(
        Conditions.any(),
        SelectOptions.withTimeout(100500).withBatchSize(1000).withFields(Arrays.asList("id"))
    )

Here SelectOptions inherit some base class that provides withTimeout and withFields methods.

P.S. I think we further will replace the single Conditions class in the methods where it is passed with a hierarchy, like PrimaryIndexConditions (or just PrimaryIndex) for the methods that support only it and full Conditions (or IndexConditions?) for select.

akudiyar · 2022-09-15T22:26:29Z

src/main/java/io/tarantool/driver/api/space/TarantoolSpaceOperations.java

+    /**
+     * Select tuples matching the specified query with specified conditions and options.
+     *
+     * @param conditions specified conditions


Let's make the doc more descriptive. query with options or something better for conditions, and more words for options.

akudiyar · 2022-09-15T22:30:17Z

src/main/java/io/tarantool/driver/core/proxy/CRUDBaseOptions.java


        public T withTimeout(int timeout) {
            this.timeout = timeout;
            return self();
        }
+
+        public T withOptions(Options options) {


I think that timeout should be a part of Options. Therefore we'll be able to specify a custom timeout for each operation. This will be also useful for the box interface.

In merged PR withTimeout is using as default parameter(to pass config.timeout) and withOptions -> timeout as custom timeout

akudiyar · 2022-09-15T22:32:22Z

src/main/java/io/tarantool/driver/core/proxy/CRUDFunctionOptions.java

+ * @author Alexey Kuzin
+ * @author Artyom Dubinin
+ */
+class CRUDFunctionOptions extends CRUDBaseOptions {


I don't like this name very much, it is confusing. And why do we have here a duplication of ProxyFunctionOptions?

In merged PR we've discussed to name CRUDReturnOptions

src/main/java/io/tarantool/driver/core/space/ProxyTarantoolSpace.java

src/main/java/io/tarantool/driver/core/space/options/Options.java

akudiyar · 2022-09-15T22:41:44Z

src/main/java/io/tarantool/driver/core/proxy/CRUDFunctionOptions.java

+/**
+ * This class is not part of the public API.
+ *
+ * Represent options for cluster proxy operations which returns fields.


Suggested change

* Represent options for cluster proxy operations which returns fields.

* Represent options for cluster proxy operations that return fields.

It has been rewritten

akudiyar · 2022-09-15T23:05:01Z

src/test/java/io/tarantool/driver/integration/ProxyTarantoolSpaceIT.java

+/**
+ * @author Artyom Dubinin
+ */
+public class ProxyTarantoolSpaceIT extends SharedCartridgeContainer {


Why do we need a new test class?

Added some classes in merged PR. I did this because the main class ProxyAPI is overloaded with many testcases, I wanted more precision test classes

ArtDu · 2022-09-16T12:23:36Z

An example of how the Options class can be used:
    space.select(
        Conditions.any(),
        SelectOptions.withTimeout(100500).withBatchSize(1000).withFields(Arrays.asList("id"))
    )
Here SelectOptions inherit some base class that provides withTimeout and withFields methods.

I see only one way how to implement this, we should have all abstract classes except leaf in inheritance hierarchy. Leaf implement self and also we should have one entrypoint in leaf, something like create.

SelectOptions.create().withTimeout(100500).withBatchSize(1000).withFields(Arrays.asList("id"))

akudiyar

Almost good, the questions are mostly about the docstrings.

akudiyar · 2022-09-20T18:10:52Z

src/main/java/io/tarantool/driver/api/space/options/OperationWithFieldsOptions.java

+    /**
+     * Return list of fields.
+     *
+     * @return timeout, in milliseconds.


Not timeout, fields

akudiyar · 2022-09-20T18:11:25Z

src/main/java/io/tarantool/driver/api/space/options/OperationWithFieldsOptions.java

+ */
+public interface OperationWithFieldsOptions extends OperationWithTimeoutOptions {
+    /**
+     * Return list of fields.


It would be better to have a more full description (what is this list for?)

akudiyar · 2022-09-20T18:20:58Z

src/main/java/io/tarantool/driver/api/space/options/proxy/ProxyReturnOptions.java

+
+    public static final String FIELDS = "fields";
+
+    /** TODO:


akudiyar · 2022-09-20T18:22:05Z

src/main/java/io/tarantool/driver/api/space/options/proxy/ProxyReturnOptions.java

+    }
+
+    @Override
+    public Optional<List> getFields() {


I think we need List<String> here

We can't have List<String> here, because we can't pass List<String>.class

Why we can't? Looks like we can change method interface methods signature

Yeah we can change method signature, but in the inner implementation we need to do a hack to cast to List<String>.class. The problem is about we are storing any option types in a Map with generic Object type

akudiyar · 2022-09-20T18:29:13Z

src/main/java/io/tarantool/driver/core/proxy/CRUDBatchOptions.java


    public static final String BATCH_STOP_ON_ERROR = "stop_on_error";
    public static final String BATCH_ROLLBACK_ON_ERROR = "rollback_on_error";

    protected
-    <T extends AbstractBuilder<T>>
-    CRUDBatchOptions(AbstractBuilder<T> builder) {
+    <O extends CRUDBatchOptions, B extends AbstractBuilder<O, B>>


CRUDBatchOptions is a final class, we don't need a generic for Options here

akudiyar · 2022-09-20T18:30:56Z

src/main/java/io/tarantool/driver/core/proxy/CRUDReturnOptions.java

+/**
+ * This class is not part of the public API.
+ * <p>
+ * Represent options for cluster proxy operations which returns fields


Let's reformulate it. Something like operations for that the set of fields returned in the result tuples can be specified?

Has been rewritten to

* Represent returned options for cluster proxy operations. * The return set of fields of the tuples can be specified.

akudiyar · 2022-09-20T18:32:30Z

src/main/java/io/tarantool/driver/core/proxy/CRUDSelectOptions.java


    public static final String SELECT_LIMIT = "first";
    public static final String SELECT_AFTER = "after";
    public static final String SELECT_BATCH_SIZE = "batch_size";

-    private <T extends AbstractBuilder<T>>
-    CRUDSelectOptions(AbstractBuilder<T> builder) {
+    private <O extends CRUDSelectOptions, B extends AbstractBuilder<O, B>>


THis class is final, the generic for Options is redundant.

ArtDu · 2022-09-20T19:20:10Z

Almost good, the questions are mostly about the docstrings.

Yeah, that is reason why I've converted PR to Draft

Elishtar · 2022-09-20T22:02:35Z

src/main/java/io/tarantool/driver/core/proxy/CRUDBaseOptions.java

-    <O extends CRUDBaseOptions, T extends AbstractBuilder<O, T>>
-    CRUDBaseOptions(AbstractBuilder<O, T> builder) {
+    <O extends CRUDBaseOptions, B extends AbstractBuilder<O, B>>
+    CRUDBaseOptions(AbstractBuilder<O, B> builder) {


I guess this case should be excluded from checkstyle only one line, current format looks not readable

Added, please check and give your opinion how to do it correctly

Elishtar · 2022-09-20T22:03:45Z

src/main/java/io/tarantool/driver/core/proxy/CRUDBaseOptions.java

-    class AbstractBuilder<O extends CRUDBaseOptions, T extends AbstractBuilder<O, T>>
-        extends CRUDAbstractOperationOptions.AbstractBuilder<O, T> {
+    class AbstractBuilder<O extends CRUDBaseOptions, B extends AbstractBuilder<O, B>>
+        extends CRUDAbstractOperationOptions.AbstractBuilder<O, B> {


I think. looks better

protected abstract static class AbstractBuilder<O extends CRUDBaseOptions, B extends AbstractBuilder<O, B>> extends CRUDAbstractOperationOptions.AbstractBuilder<O, B> { protected Optional<Integer> timeout = Optional.empty();

src/main/java/io/tarantool/driver/core/proxy/CRUDBaseOptions.java

Elishtar · 2022-09-20T22:08:24Z

src/main/java/io/tarantool/driver/core/proxy/CRUDReturnOptions.java

+    protected <O extends CRUDReturnOptions, B extends AbstractBuilder<O, B>>
+    CRUDReturnOptions(CRUDReturnOptions.AbstractBuilder<O, B> builder) {
+        super(builder);
+


Remove space

ArtDu · 2022-09-23T15:09:20Z

We have one problem with our approach we can't have multiple inheritances in java and especially in our ProxyOptions.
For example, we have InsertOptions and CountOption, both have bucket_id option, but only InsertOption has fields option

For these cases, we need to duplicate the code.

ArtDu · 2022-09-26T12:28:54Z

Blocked by #272 because driver should parse metadata from crud. We can't get field by field name if we don't know position and It will frustrate users

Closes #236

akudiyar requested changes Sep 15, 2022

View reviewed changes

ArtDu force-pushed the crud/fields_v2 branch from 314b478 to 2b0a3d3 Compare September 17, 2022 22:02

ArtDu changed the base branch from refactor-crud-operation-options to master September 17, 2022 22:02

ArtDu changed the title ~~Add 'fields' parameter to ProxySpace v2~~ Add 'fields' parameter to ProxySpace Sep 17, 2022

ArtDu force-pushed the crud/fields_v2 branch from 2b0a3d3 to 57f8356 Compare September 19, 2022 21:26

ArtDu marked this pull request as ready for review September 19, 2022 21:40

ArtDu force-pushed the crud/fields_v2 branch from 57f8356 to a15eed3 Compare September 19, 2022 21:40

ArtDu requested review from akudiyar and Elishtar September 19, 2022 21:40

ArtDu force-pushed the crud/fields_v2 branch 2 times, most recently from 894e8a3 to 1618cfe Compare September 20, 2022 17:58

ArtDu marked this pull request as draft September 20, 2022 17:59

akudiyar requested changes Sep 20, 2022

View reviewed changes

Elishtar reviewed Sep 20, 2022

View reviewed changes

src/main/java/io/tarantool/driver/core/proxy/CRUDBaseOptions.java Show resolved Hide resolved

Elishtar reviewed Sep 20, 2022

View reviewed changes

ArtDu force-pushed the crud/fields_v2 branch from db9c4f4 to 0ce821a Compare September 22, 2022 14:12

ArtDu marked this pull request as ready for review September 22, 2022 14:17

ArtDu requested review from akudiyar and Elishtar September 22, 2022 14:17

ArtDu force-pushed the crud/fields_v2 branch from 0ce821a to d82b697 Compare September 23, 2022 10:54

Elishtar previously approved these changes Sep 23, 2022

View reviewed changes

ArtDu marked this pull request as draft September 26, 2022 12:11

ArtDu added the blocked Not ready to be implemented label Sep 26, 2022

ArtDu dismissed Elishtar’s stale review via 00857c2 December 21, 2022 13:53

ArtDu force-pushed the crud/fields_v2 branch 2 times, most recently from 00857c2 to 77f358d Compare December 21, 2022 14:09

ArtDu removed the blocked Not ready to be implemented label Dec 21, 2022

ArtDu marked this pull request as ready for review December 21, 2022 14:23

ArtDu mentioned this pull request Dec 21, 2022

Use options like mixins in proxy operations #330

Merged

5 tasks

ArtDu requested a review from Elishtar December 21, 2022 14:25

Add fields option to ProxySpace

87f9179

Closes #236

ArtDu force-pushed the crud/fields_v2 branch from 77f358d to 87f9179 Compare December 22, 2022 08:18

Elishtar approved these changes Dec 22, 2022

View reviewed changes

ArtDu merged commit 9ef0f47 into master Dec 22, 2022

ArtDu deleted the crud/fields_v2 branch December 22, 2022 09:18

	* Represent options for cluster proxy operations which returns fields.
	* Represent options for cluster proxy operations that return fields.

Add 'fields' parameter to ProxySpace #263

Add 'fields' parameter to ProxySpace #263

Uh oh!

Conversation

ArtDu commented Sep 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

akudiyar 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!

ArtDu Sep 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akudiyar Sep 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ArtDu commented Sep 16, 2022

Uh oh!

akudiyar 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!

ArtDu Sep 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ArtDu Sep 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

ArtDu commented Sep 20, 2022

Uh oh!

Choose a reason for hiding this comment

ArtDu commented Sep 15, 2022 •

edited

Loading

ArtDu Sep 19, 2022 •

edited

Loading

akudiyar Sep 15, 2022 •

edited

Loading

ArtDu Sep 22, 2022 •

edited

Loading

ArtDu Sep 23, 2022 •

edited

Loading

Elishtar Sep 20, 2022 •

edited

Loading

ArtDu commented Sep 23, 2022 •

edited

Loading