Skip to content

Conversation

ArtDu
Copy link
Contributor

@ArtDu ArtDu commented Apr 14, 2023

Before changing:

Benchmark                   Mode  Cnt      Score       Error  Units
BenchmarkRunner.spaceCall  thrpt    5  74844.508 ± 16460.492  ops/s

After changing:

Benchmark                   Mode  Cnt       Score        Error  Units
BenchmarkRunner.spaceCall  thrpt    5  275476.264 ± 256775.771  ops/s
  • Error means mean margin between iteration:

This is the margin of error for the score. In most cases, that is a half of confidence interval. Think about it as if there is a "±" sign between "Score" and "Score error". In fact, the human-readable log will show that:

Iteration   1: 334564.445 ops/s
Iteration   2: 226016.924 ops/s
Iteration   3: 343260.667 ops/s
Iteration   4: 190627.608 ops/s
Iteration   5: 282911.676 ops/s

I haven't forgotten about:

  • Tests
  • Changelog
  • Documentation
  • Commit messages comply with the guideline
  • Cleanup the code for review. See checklist

Related issues:
Closes #382

@ArtDu ArtDu force-pushed the double_executor branch from 46b7eec to 432a565 Compare May 3, 2023 14:53
@ArtDu ArtDu force-pushed the double_executor branch from 432a565 to 46e2d8d Compare May 3, 2023 14:58
@ArtDu ArtDu marked this pull request as ready for review May 3, 2023 14:58
@ArtDu ArtDu changed the title WIP: Get rid of double executor in retrying Get rid of double executor in retrying May 3, 2023
@ArtDu ArtDu requested review from akudiyar, bitgorbovsky and dkasimovskiy and removed request for dkasimovskiy May 3, 2023 14:59
.build();

retryingTarantoolClient
= TarantoolClientFactory.configureClient(tarantoolClient).withRetryingByNumberOfAttempts(3).build();
Copy link

@vasilevichra vasilevichra May 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Such a long name. Why not withRetry(3) or withAttempt(42) or withMaxTries(99)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, maybe we should rename it. Could you create a ticket for it?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a builder. We can make it hierarchical, but with plain structure as it is now such a long name is necessary because it specifies two options in one time: that we have a retry mechanism enabled and that we specify number of attempts as its parameter.

Copy link
Collaborator

@akudiyar akudiyar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

.build();

retryingTarantoolClient
= TarantoolClientFactory.configureClient(tarantoolClient).withRetryingByNumberOfAttempts(3).build();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a builder. We can make it hierarchical, but with plain structure as it is now such a long name is necessary because it specifies two options in one time: that we have a retry mechanism enabled and that we specify number of attempts as its parameter.

@akudiyar akudiyar added this pull request to the merge queue May 3, 2023
Merged via the queue into master with commit 8ca1eea May 3, 2023
@akudiyar akudiyar deleted the double_executor branch May 3, 2023 19:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove double executor run from RetryingClient.space
3 participants