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

Add Snapshot-read-only isolation support #79

Merged
merged 1 commit into from
Jun 27, 2024

Conversation

jorki02
Copy link
Contributor

@jorki02 jorki02 commented Jun 26, 2024

You can read more about this type of isolation here https://ydb.tech/docs/en/concepts/transactions#modes

@nvamelichev nvamelichev self-requested a review June 26, 2024 15:13
@nvamelichev
Copy link
Collaborator

We'll need a basic test: just check that saving an entity in serializable RW mode and then reading in a snapshot transaction works. Please add this to YdbIntegrationTest. You'll need a locally running YDB-in-Docker to run YdbIntegrationTest, here's what config YOJ historically uses (2135 plaintext YDB port) 🦔

docker run -i -p 2135:2135 -p 2136:2136 -p 8765:8765 -e YDB_USE_IN_MEMORY_PDISKS=true -e YDB_KQP_RESULT_ROWS_LIMIT=10000 -e GRPC_PORT=2135 -e GRPC_TLS_PORT=2136 -e MON_PORT=8765 --hostname localhost --rm --name ydb cr.yandex/yc/yandex-docker-local-ydb:latest

(Don't forget to docker kill ydb after you are done testing.)

Otherwise LGTM 👍

Copy link
Collaborator

@nvamelichev nvamelichev left a comment

Choose a reason for hiding this comment

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

All good! 👍

@nvamelichev nvamelichev merged commit 77de80e into ydb-platform:main Jun 27, 2024
1 check passed
@lavrukov
Copy link
Contributor

lavrukov commented Jul 2, 2024

@jorki02 Sorry, but I have to revert this changes. SnapshotRO - it's isolation level similar to serializableRw, but with additional validation. It means that this transactions have to be commited, but txManager.readOnly() don't do it.

If you don't do it, YDB will close transactions for you due to a timeout, but if 10 transactions are opened within one session, new ones will no longer be able to be opened.

This feature should be added by create new method txManager.snapshotRo()

@jorki02
Copy link
Contributor Author

jorki02 commented Jul 3, 2024

@lavrukov Hi, why do you think that it is not committed on readOnly?
Is see that readOnly builder uses https://github.com/ydb-platform/yoj-project/blob/main/repository/src/main/java/tech/ydb/yoj/repository/db/StdTxManager.java#L404
Which is evantually committed here https://github.com/ydb-platform/yoj-project/blob/main/repository/src/main/java/tech/ydb/yoj/repository/db/TxImpl.java#L96
Currently, this isolation works on our production, and it has much more then 10 transactions per session, and I didn't notice any problem with this code.

@jorki02
Copy link
Contributor Author

jorki02 commented Jul 3, 2024

Oh, I see it's committed, but not committed)
https://github.com/ydb-platform/yoj-project/blob/main/repository-ydb-v2/src/main/java/tech/ydb/yoj/repository/ydb/YdbRepositoryTransaction.java#L209
Probably better solution wuold be to add one more "if" here https://github.com/ydb-platform/yoj-project/blob/main/repository-ydb-v2/src/main/java/tech/ydb/yoj/repository/ydb/YdbRepositoryTransaction.java#L185 ?
Strange that I didn't see any problem with it for around 2 month, when I used fork with this change

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.

3 participants