Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Provide an API to get storages initialization state #299

Merged
merged 2 commits into from
Jul 27, 2022

Conversation

psergee
Copy link
Contributor

@psergee psergee commented Jun 2, 2022

There is an issue with using CRUD functionality if not all storages are
up. New function is added to get the information about storages
state: initialized or not. So, a user can poll state and wait for
storages to be initialized before making CRUD calls.

Looks like state.lua module is unused. Removed.

Resolves #229

Copy link
Member

@DifferentialOrange DifferentialOrange left a comment

Choose a reason for hiding this comment

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

Original ticket discusses some possibility to implement simple retries, while this solution provide a manual handle to check the state. Was this design approved by issue authors or product managers? Also, to be honest, I don't think current approach to state check is satisfying (see comments).

CHANGELOG.md Outdated Show resolved Hide resolved
crud/common/const.lua Show resolved Hide resolved
crud/common/state.lua Show resolved Hide resolved
crud/common/utils.lua Show resolved Hide resolved
crud/common/utils.lua Outdated Show resolved Hide resolved
@psergee psergee force-pushed the psergee/gh-229-crud-storage-init-status branch from f32ad8e to 3bf2262 Compare June 22, 2022 11:23
@psergee
Copy link
Contributor Author

psergee commented Jun 22, 2022

Original ticket discusses some possibility to implement simple retries, while this solution provide a manual handle to check the state. Was this design approved by issue authors or product managers? Also, to be honest, I don't think current approach to state check is satisfying (see comments).

We have discussed this approach with Java connector team and @Totktonada
Currently we could not come up with a better option to provide the information about CRUD storage init state. Timeout approach does not look useful, because storage start may take a long time and CRUD calls like select or get will "hang" if timeout is big enough. In case of small timeout, the original issue will not fade away.

Copy link
Member

@DifferentialOrange DifferentialOrange left a comment

Choose a reason for hiding this comment

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

Thank you, LGTM!

I'll merge it after #232 . Maybe you'll need to rebase this PR and solve remaining conflict before merge.

CHANGELOG.md Outdated Show resolved Hide resolved
crud.lua Outdated Show resolved Hide resolved
crud/common/utils.lua Outdated Show resolved Hide resolved
crud/common/utils.lua Outdated Show resolved Hide resolved
crud/common/utils.lua Outdated Show resolved Hide resolved
crud/common/utils.lua Outdated Show resolved Hide resolved
crud/common/utils.lua Outdated Show resolved Hide resolved
crud/common/utils.lua Outdated Show resolved Hide resolved
@Totktonada
Copy link
Member

Original ticket discusses some possibility to implement simple retries, while this solution provide a manual handle to check the state. Was this design approved by issue authors or product managers?

Discussed between me, Pavel. S and cartridge-java developers. We agreed that a function that will return status of storages will suit the needs.

I can share my vision of the problem. There are a bit different situations:

  1. Waiting for the cluster boostrap (full or partial).
  2. Run queries.

There are different possible mechanisms that can help in those situations:

  1. A status endpoint.
  2. Timeouts for queries.
  3. Contract on errors that allows a client to spot the non-bootstrapped storage situation and retry.

Timeouts for queries is the most used mechanish and it allows to overcome small periods of a peer unavailability. We support it, but there are flaws: #95.

The also unified error messages for the non-bootstrapped storage case, see PR #296 and PR #297. (It is not great that we propose to match the error message to make programmatic decisions, but errors in crud -- and even in tarantool itself -- requires a redesign and rework IMHO. At least, some revision.)

However we still don't offer anything to wait for a potentially long times (over usual query timeout) until the cluster will go into the ready state. It is what we want to solve here.

@psergee psergee force-pushed the psergee/gh-229-crud-storage-init-status branch 3 times, most recently from 58633d9 to 3035210 Compare June 28, 2022 13:05
@psergee psergee force-pushed the psergee/gh-229-crud-storage-init-status branch from 3035210 to 428b629 Compare July 1, 2022 12:37
@Totktonada Totktonada self-requested a review July 1, 2022 14:28
@psergee psergee force-pushed the psergee/gh-229-crud-storage-init-status branch 2 times, most recently from 04f1aba to f3aa7c9 Compare July 11, 2022 08:11
Copy link
Member

@Totktonada Totktonada left a comment

Choose a reason for hiding this comment

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

Thanks! The patchset is okay for me in general. However, please, consider several comments and thoughts.

crud/common/utils.lua Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
crud.lua Outdated Show resolved Hide resolved
crud/common/utils.lua Outdated Show resolved Hide resolved
test/integration/storages_state_test.lua Outdated Show resolved Hide resolved
test/integration/storages_state_test.lua Outdated Show resolved Hide resolved
@psergee psergee force-pushed the psergee/gh-229-crud-storage-init-status branch from f3aa7c9 to fcefce9 Compare July 13, 2022 14:49
crud/common/utils.lua Outdated Show resolved Hide resolved
@psergee psergee force-pushed the psergee/gh-229-crud-storage-init-status branch from fcefce9 to 41f42cc Compare July 14, 2022 07:25
README.md Outdated Show resolved Hide resolved
test/integration/storages_state_test.lua Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
crud/common/utils.lua Outdated Show resolved Hide resolved
test/integration/storages_state_test.lua Outdated Show resolved Hide resolved
test/integration/storages_state_test.lua Outdated Show resolved Hide resolved
test/integration/storages_state_test.lua Outdated Show resolved Hide resolved
test/integration/storages_state_test.lua Outdated Show resolved Hide resolved
test/integration/storages_state_test.lua Outdated Show resolved Hide resolved
test/integration/storages_state_test.lua Outdated Show resolved Hide resolved
@DifferentialOrange DifferentialOrange self-requested a review July 19, 2022 12:10
@psergee psergee force-pushed the psergee/gh-229-crud-storage-init-status branch 2 times, most recently from dcda205 to 25c6594 Compare July 21, 2022 13:44
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@psergee psergee force-pushed the psergee/gh-229-crud-storage-init-status branch 2 times, most recently from e9c58bf to 647dba9 Compare July 22, 2022 08:24
@DifferentialOrange
Copy link
Member

DifferentialOrange commented Jul 24, 2022

@psergee psergee force-pushed the psergee/gh-229-crud-storage-init-status branch from 647dba9 to f4792ea Compare July 25, 2022 10:30
There is an issue with using CRUD functionality if not all storages are
up. New function is added to get the information about storages
state: initialized or not. So, a user can poll state and wait for
storages to be initialized before making CRUD calls.

Resolves #229
@psergee psergee force-pushed the psergee/gh-229-crud-storage-init-status branch from f4792ea to 9a730ca Compare July 26, 2022 11:55
@DifferentialOrange DifferentialOrange merged commit d053623 into master Jul 27, 2022
@DifferentialOrange DifferentialOrange deleted the psergee/gh-229-crud-storage-init-status branch July 27, 2022 07:25
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.

Calls to routers and storages when crud-router and crud-storage roles initialization is not finished yet
3 participants