Make CRUDOptions use interface to reduce code duplications by nickkkccc · Pull Request #418 · tarantool/cartridge-java

nickkkccc · 2023-09-10T21:45:51Z

I haven't forgotten about:

Related issues:

Closes #417

ArtDu

Speaking about enum and wrappers.
First of all, it is a breaking change.
The second one is why we use wrapper before enum. It makes code more complicated.

-.withMode(new ModeValue(ModeValue.WRITE))
+.withMode(ModeValue.WRITE)

I'm asking you to separate changes on two pull request:

"Make CRUDOptions use interface to reduce code duplications"
"Use enum for proxy client parameters"

src/main/java/io/tarantool/driver/api/space/options/crud/enums/CRUDOption.java

src/test/java/io/tarantool/driver/integration/proxy/options/ProxySpaceUpdateOptionsIT.java

akudiyar

Overall: let's not introduce new entities (except enums) if the values do not have any special operations defined on them. IIf there is a strict necessity to do that, let's then

not make the user write new every time (use factory methods like MyClass.of(value))
allow the user to use simple (built-in) types where it is possible

Anyway, any such changes are breaking so let's discuss them first. We will need to explain that in a doc and ideally add a memo/page in the wiki to show the users how to fix their code after the update.

Regarding the option names: having CRUD in them is logical, but let's therefore make them easily separable from the rest driver code to eventually make a new library package only for the tarantool/crud support.

...in/java/io/tarantool/driver/api/space/options/crud/contracts/OperationWithFieldsOptions.java

src/main/java/io/tarantool/driver/api/space/options/crud/enums/CRUDOption.java

src/test/java/io/tarantool/driver/integration/proxy/options/ProxySpaceInsertOptionsIT.java

Add OperationWithAfterOptions and OperationWithFirstOptions classes. Needed for #417.

- Add OperationWIthBatchSizeOptions class. - Change ProxySelectOptions, SelectOptions classes. - Add package-info. Needed for #417.

Add asMap() general method in BaseOptions, Options classes. Needed for #417.

- Remove CRUD<name>Options classes like a duplicates. - Add options directly to the argument list without CRUD<name>Options class builders. - Change Map:toString() test in the ProxyOperationBuildersTest class with EnumMap specific. - Add test in ProxySpaceSelectOptionsIT class with after and first options. Closes #417.

Solved

nickkkccc · 2023-09-13T23:52:29Z

I think I was able to get rid of the duplication of the CRUDOptions classes (identical to the ProxyOptions classes) by moving the options directly into the argument list.

ArtDu

We need to think how these changes modify cluster client working pipeline

CHANGELOG.md

ArtDu · 2023-09-15T08:27:14Z

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

        return Optional.ofNullable((T) resultMap.get(option));
    }
+
+    public Map<String, Object> asMap() {


Do we need cope of the map only for prechecking null values? If it's true we can add null checking in addOption method and remove copying

I think we need an immutable map here

ArtDu · 2023-09-15T08:32:50Z

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

 import io.tarantool.driver.api.SingleValueCallResult;
 import io.tarantool.driver.api.TarantoolCallOperations;
-import io.tarantool.driver.api.space.options.Options;
+import io.tarantool.driver.api.space.options.interfaces.Options;


If this change involves user API we need to add it as breaking change in changelog

Needed for #417.

- Add OperationWith<option-name>Options contract interfaces. - Add ProxyOperationArgument enum class for parts of arguments list. - Change <operation-name>ProxyOperation classes for using with contract interfaces. - Move ProxyOperation interface from `io.tarantool.driver.core.proxy` package to `io.tarantool.driver.core.proxy.interfaces` package. Closes #417.

akudiyar

Almost good, however I don't get why we need to change the package names. Would you mind elaborating a bit about the purpose of the new packages? A lines-and-boxes diagram may be useful

akudiyar · 2023-09-19T11:40:48Z

CHANGELOG.md

- Add "mode" option for crud select operation ([#107](https://github.com/tarantool/cartridge-java/issues/107))
- Change using of proxy client parameters (mode, rollback_on_error, stop_on_error) with enum classes ([#419](https://github.com/tarantool/cartridge-java/issues/419))
-
+- Add `"mode"` option for crud select operation ([#107](https://github.com/tarantool/cartridge-java/issues/107))


Just "select operation" perhaps? Is we are actually making unified API not only for tarantool/crud

Good point.

Just "select operation" perhaps? Is we are actually making unified API not only for tarantool/crud

Yeap, we need to separate options between proxy and cluster clients. For example I suggest to make SelectOptions without API extending only Options interface(addOption/getOption and asMap methods) because we use only these methods in inner API. But inheritance interfaces will involve specific methods like mode.

Options (interface) | V SelectOptions (interface) | | V V ProxySelectOptions (interface) ClusterSelectOptions (interface)

akudiyar · 2023-09-19T11:41:21Z

src/main/java/io/tarantool/driver/api/TarantoolClient.java

 import io.tarantool.driver.api.metadata.TarantoolMetadataOperations;
 import io.tarantool.driver.api.metadata.TarantoolMetadataProvider;
-import io.tarantool.driver.api.space.TarantoolSpaceOperations;
+import io.tarantool.driver.api.space.options.interfaces.TarantoolSpaceOperations;


Why do we need to make the packet names longer?

I wanted a more organized arrangement of interfaces and classes that implement those interfaces. A contract is a contract that interfaces-options expose to classes that implement it.

All this .api. package is about contracts. Adding more contract sub-layers here seems an overkill

akudiyar · 2023-09-19T11:42:06Z

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

        return Optional.ofNullable((T) resultMap.get(option));
    }
+
+    public Map<String, Object> asMap() {


I think we need an immutable map here

akudiyar · 2023-09-19T11:43:35Z

src/main/java/io/tarantool/driver/api/space/options/contracts/OperationWithBucketIdOptions.java

@@ -1,6 +1,8 @@
-package io.tarantool.driver.api.space.options;
+package io.tarantool.driver.api.space.options.contracts;


What does this new package name mean, what does it give to the users?

akudiyar · 2023-09-19T11:47:44Z

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


            return new DeleteProxyOperation<>(
-                this.client, this.functionName, arguments, this.argumentsMapper, this.resultMapper);
+                this.client, this.functionName, new ArrayList<>(arguments.values()), this.argumentsMapper,


Why do we have that additional wrapping here? It will result in significant overhead for memory and GC time at least

Wrapping in enummap was required to make unification across interfaces possible. If we don't use enummap, how can we connect the parts of the argument list in the correct number and order? After all, for each operation there are parts of the argument list that are different in composition and order.

I mean the new ArrayList<>() call first of all

I realize that calling new ArrayList<>() creates intermediate collections. But it seems to me that the same collection is created before, which is later referenced by the "final" argument list:

cartridge-java/src/main/java/io/tarantool/driver/core/proxy/DeleteProxyOperation.java

Lines 57 to 64 in b743fe5

List<?> arguments = Arrays.asList(spaceName, indexQuery.getKeyValues(), requestOptions.asMap());

return new DeleteProxyOperation<>(

this.client, this.functionName, arguments, this.argumentsMapper, this.resultMapper);

}

}

}

Either I don't have enough experience to understand the difference between the two cases:
Master case:

cartridge-java/src/main/java/io/tarantool/driver/core/proxy/DeleteProxyOperation.java

Lines 57 to 64 in b743fe5

List<?> arguments = Arrays.asList(spaceName, indexQuery.getKeyValues(), requestOptions.asMap());

return new DeleteProxyOperation<>(

this.client, this.functionName, arguments, this.argumentsMapper, this.resultMapper);

}

}

}

Commit case:

cartridge-java/src/main/java/io/tarantool/driver/core/proxy/DeleteProxyOperation.java

Lines 47 to 53 in 670297e

return new DeleteProxyOperation<>(

this.client, this.functionName, new ArrayList<>(arguments.values()), this.argumentsMapper,

this.resultMapper);

}

}

}

akudiyar · 2023-09-19T11:49:39Z

src/main/java/io/tarantool/driver/core/proxy/contracts/package-info.java

+ *
+ * @author <a href="https://github.com/nickkkccc">Belonogov Nikolay</a>
+ */
+package io.tarantool.driver.core.proxy.contracts;


Do we really need that complex package structure? I haven't seen "contracts" packages in other libraries.

...main/java/io/tarantool/driver/api/space/options/contracts/OperationWIthBatchSizeOptions.java

ArtDu

Good job on improvement our complex API. I wrote some comments. It will be the best if you also create tickets about my notions in java doc

src/main/java/io/tarantool/driver/core/proxy/enums/ProxyOperationArgument.java

src/main/java/io/tarantool/driver/api/space/options/interfaces/Options.java

src/main/java/io/tarantool/driver/api/space/options/interfaces/SelectOptions.java

...main/java/io/tarantool/driver/api/space/options/contracts/OperationWIthBatchSizeOptions.java

- Fix `API changes` description. - Fix mistype and typo in `OperationWithBatchSizeOptions` interface. - Add notions to `Options`, `ProxyOperationArgument`, `SelectOptions`. Closes #417.

nickkkccc · 2023-09-19T15:32:45Z

@akudiyar, we decided not to make Map immutable for now. If necessary, we will change it in the next stages.

akudiyar

Let's not introduce new packages like interfaces and contracts since we are already in the api package that means exactly that. Also think of the amount of changes the users will have to make in their code, will these changes be meaningful?

Also be careful about creating new objects on each call, new ArrayList<> for example is not necessary

akudiyar

See my comment above - let's discuss that verbally if you wish

Closes #417.

nickkkccc linked an issue Sep 10, 2023 that may be closed by this pull request

Make CRUDOptions use interface to reduce code duplications #417

Closed

nickkkccc requested review from ArtDu and akudiyar September 10, 2023 21:47

ArtDu reviewed Sep 11, 2023

View reviewed changes

src/main/java/io/tarantool/driver/api/space/options/crud/enums/CRUDOption.java Outdated Show resolved Hide resolved

src/test/java/io/tarantool/driver/integration/proxy/options/ProxySpaceUpdateOptionsIT.java Outdated Show resolved Hide resolved

nickkkccc changed the title ~~Make CRUDOptions unified~~ Make CRUDOptions use interface to reduce code duplications Sep 11, 2023

akudiyar previously requested changes Sep 11, 2023

View reviewed changes

nickkkccc added 4 commits September 13, 2023 13:25

Refactor in space package

f7fe293

Add interfaces for after and first options

9bd734c

Add OperationWithAfterOptions and OperationWithFirstOptions classes. Needed for #417.

Change batch_size option interfaces

258a00b

- Add OperationWIthBatchSizeOptions class. - Change ProxySelectOptions, SelectOptions classes. - Add package-info. Needed for #417.

Add asMap() general method

d643df8

Add asMap() general method in BaseOptions, Options classes. Needed for #417.

nickkkccc force-pushed the gh-417 branch from 8d15c47 to 1753d3e Compare September 13, 2023 23:21

nickkkccc force-pushed the gh-417 branch from 1753d3e to c8bfc3f Compare September 13, 2023 23:42

nickkkccc requested review from ArtDu and akudiyar September 13, 2023 23:46

nickkkccc self-assigned this Sep 13, 2023

ArtDu reviewed Sep 15, 2023

View reviewed changes

nickkkccc added 4 commits September 18, 2023 11:45

Remove after, first contract interfaces

59a305d

Needed for #417.

Add breaking change to CHANGELOG.md

e7cd3e9

Needed for #417.

Remove duplication for options map

f16d9ca

Needed for #417.

nickkkccc requested a review from ArtDu September 18, 2023 17:04

ArtDu marked this pull request as ready for review September 19, 2023 10:04

akudiyar requested changes Sep 19, 2023

View reviewed changes

ArtDu requested changes Sep 19, 2023

View reviewed changes

nickkkccc force-pushed the gh-417 branch from 1238e6a to 92afcfb Compare September 19, 2023 13:36

nickkkccc requested a review from ArtDu September 19, 2023 13:37

nickkkccc requested a review from akudiyar September 19, 2023 13:39

Fix CHANGELOG.md and mistype, add notions

670297e

- Fix `API changes` description. - Fix mistype and typo in `OperationWithBatchSizeOptions` interface. - Add notions to `Options`, `ProxyOperationArgument`, `SelectOptions`. Closes #417.

nickkkccc force-pushed the gh-417 branch from de73723 to 670297e Compare September 19, 2023 14:04

ArtDu previously approved these changes Sep 19, 2023

View reviewed changes

akudiyar requested changes Sep 20, 2023

View reviewed changes

nickkkccc requested a review from akudiyar September 21, 2023 07:59

akudiyar requested changes Sep 21, 2023

View reviewed changes

Fix unnecessary complexity of packages

5d70a80

Closes #417.

nickkkccc dismissed ArtDu’s stale review via 5d70a80 September 25, 2023 22:32

nickkkccc requested a review from ArtDu September 25, 2023 22:36

dkasimovskiy merged commit 78a66d9 into master Sep 26, 2023

dkasimovskiy deleted the gh-417 branch September 26, 2023 07:24

akudiyar mentioned this pull request Oct 3, 2023

Fix incorrect package change for TarantoolSpaceOperations #430

Merged

		@@ -1,6 +1,8 @@
		package io.tarantool.driver.api.space.options;
		package io.tarantool.driver.api.space.options.contracts;


	List<?> arguments = Arrays.asList(spaceName, indexQuery.getKeyValues(), requestOptions.asMap());

	return new DeleteProxyOperation<>(
	this.client, this.functionName, arguments, this.argumentsMapper, this.resultMapper);
	}
	}
	}


	return new DeleteProxyOperation<>(
	this.client, this.functionName, new ArrayList<>(arguments.values()), this.argumentsMapper,
	this.resultMapper);
	}
	}
	}

Conversation

nickkkccc commented Sep 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ArtDu left a comment • 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!

Uh oh!

Uh oh!

akudiyar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nickkkccc commented Sep 13, 2023

Uh oh!

ArtDu left a comment

Choose a reason for hiding this comment

Uh oh!

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!

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!

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!

ArtDu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nickkkccc commented Sep 19, 2023

Uh oh!

akudiyar left a comment

Choose a reason for hiding this comment

Uh oh!

akudiyar left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

nickkkccc commented Sep 10, 2023 •

edited

Loading

ArtDu left a comment •

edited

Loading