Add default client request timeout #362

ArtDu · 2023-02-27T19:39:33Z

@ akudiyar changed the behaviour 57af227#diff-b2ae385ada02a75ad946141c7af41540970760d41d02d54209c00eabdc9bc8e3L175

I suggest make "OR" condition. If we have options.timeout, we use it, otherwise we use client.getTimeout()

I haven't forgotten about:

Related issues:

akudiyar · 2023-02-27T21:12:26Z

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

@@ -89,6 +89,10 @@ public void withTimeout() throws ExecutionException, InterruptedException {
        List<?> crudDeleteOpts = client.eval("return crud_delete_opts").get();
        assertEquals(requestConfigTimeout, ((HashMap) crudDeleteOpts.get(0)).get("timeout"));

+        profileSpace.delete(conditions, ProxyDeleteOptions.create()).get();
+        crudDeleteOpts = client.eval("return crud_delete_opts").get();
+        assertEquals(requestConfigTimeout, ((HashMap) crudDeleteOpts.get(0)).get("timeout"));


Seems that setting the request timeout is missing here

That also applies to all the other tests

akudiyar · 2023-02-27T21:15:23Z

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

            CRUDBucketIdOptions requestOptions = new CRUDDeleteOptions.Builder()
-                .withTimeout(options.getTimeout())
+                .withTimeout(optionsTimeout.isPresent() ? optionsTimeout : requestTimeout)


I'm not sure that we need two ways of setting the timeout. For what the other timeout (in options) then stands for? In the previous variant, the request timeout was passed to the response handler. I believe that we need to make them distinct timeouts and pass both of them in the options.

Passing in the options is optional, but passing to the handler is mandatory I think

But the main think about the changes that we have passing config.requestTimeout

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

Lines 251 to 256 in 97f2a19

@Override

public CompletableFuture<R> select(Conditions conditions) throws TarantoolClientException {

return select(conditions, rowsMetadataTupleResultMapper(), ProxySelectOptions.create()

.withTimeout(config.getRequestTimeout())

);

}

For example if we used fields options we wouldn't have this feature and it might frustrate. This is the thing that @vlalykin complained:

добрый день.
по поводу возможности задания списка полей в options: #236

если я задаю новый параметр для уточнения списка полей, то не передается дефолтный таймаут (который раньше задавался один раз на уровне клиента). теперь его надо каждый раз руками задавать, или я что то неправильно делаю ?

client.getSpace(SPACE).select(condition, ProxySelectOptions.create().withFields(fields))

The timeout that is passed currently to the options is different. We need to not pass the requestTimeout instead of that timeout, but rather pass this requestTimeout to the request handler as it was done previously (that's what the users want).

We always pass requestTimeout in crud option. See 0.8.0 version(it's before my options changes). Do you want to change this behavior?

Ah, it was broken since even then. Yes, I think that we need to pass the requestTimeout to Netty, and only the timeout in the options to tarantool/crud.

akudiyar

We need to return back the previous behavior, it is currently broken. The actual changes are doing something other.

akudiyar

We need to pass requestTimeout to the handlers and timeout in options to crud

ArtDu · 2023-03-16T20:26:38Z

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

@@ -275,7 +265,6 @@ private CompletableFuture<R> select(
            .withSpaceName(spaceName)
            .withFunctionName(operationsMapping.getSelectFunctionName())
            .withConditions(conditions)
-            .withOptions(options)


akudiyar

LGTM, as it at least removes the ambiguity.

I've created a new issue #367 for implementing the requestTimeout option correctly.

ArtDu requested a review from akudiyar February 27, 2023 19:47

ArtDu marked this pull request as ready for review February 27, 2023 19:47

akudiyar requested changes Feb 27, 2023

View reviewed changes

ArtDu requested a review from akudiyar February 28, 2023 13:42

akudiyar requested changes Mar 1, 2023

View reviewed changes

ArtDu requested a review from akudiyar March 2, 2023 14:23

akudiyar requested changes Mar 3, 2023

View reviewed changes

Elishtar previously approved these changes Mar 6, 2023

View reviewed changes

ArtDu dismissed Elishtar’s stale review via 3acf5f1 March 16, 2023 20:19

ArtDu force-pushed the crud/timeout branch from 6e400f6 to 3acf5f1 Compare March 16, 2023 20:19

ArtDu commented Mar 16, 2023

View reviewed changes

ArtDu force-pushed the crud/timeout branch 2 times, most recently from 67c0825 to 4d106ef Compare March 16, 2023 20:30

ArtDu marked this pull request as draft March 16, 2023 20:34

Remove config timeout as parameter in crud client

e8736a1

ArtDu force-pushed the crud/timeout branch from 4d106ef to e8736a1 Compare March 16, 2023 20:39

ArtDu marked this pull request as ready for review March 16, 2023 20:51

ArtDu requested a review from akudiyar March 16, 2023 20:51

akudiyar approved these changes Mar 16, 2023

View reviewed changes

akudiyar added this pull request to the merge queue Mar 16, 2023

Merged via the queue into master with commit b961c62 Mar 16, 2023

akudiyar deleted the crud/timeout branch March 16, 2023 22:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add default client request timeout #362

Add default client request timeout #362

ArtDu commented Feb 27, 2023 •

edited

Loading

akudiyar Feb 27, 2023

akudiyar Feb 27, 2023

akudiyar Feb 27, 2023

akudiyar Feb 27, 2023

ArtDu Feb 28, 2023

akudiyar Mar 1, 2023

ArtDu Mar 2, 2023

akudiyar Mar 3, 2023 •

edited

Loading

akudiyar left a comment

akudiyar left a comment

ArtDu Mar 16, 2023 •

edited

Loading

akudiyar left a comment

	@Override
	public CompletableFuture<R> select(Conditions conditions) throws TarantoolClientException {
	return select(conditions, rowsMetadataTupleResultMapper(), ProxySelectOptions.create()
	.withTimeout(config.getRequestTimeout())
	);
	}

Add default client request timeout #362

Add default client request timeout #362

Conversation

ArtDu commented Feb 27, 2023 • edited Loading

akudiyar Feb 27, 2023

Choose a reason for hiding this comment

akudiyar Feb 27, 2023

Choose a reason for hiding this comment

akudiyar Feb 27, 2023

Choose a reason for hiding this comment

akudiyar Feb 27, 2023

Choose a reason for hiding this comment

ArtDu Feb 28, 2023

Choose a reason for hiding this comment

akudiyar Mar 1, 2023

Choose a reason for hiding this comment

ArtDu Mar 2, 2023

Choose a reason for hiding this comment

akudiyar Mar 3, 2023 • edited Loading

Choose a reason for hiding this comment

akudiyar left a comment

Choose a reason for hiding this comment

akudiyar left a comment

Choose a reason for hiding this comment

ArtDu Mar 16, 2023 • edited Loading

Choose a reason for hiding this comment

akudiyar left a comment

Choose a reason for hiding this comment

ArtDu commented Feb 27, 2023 •

edited

Loading

akudiyar Mar 3, 2023 •

edited

Loading

ArtDu Mar 16, 2023 •

edited

Loading