search

ydb-platform / yoj-project

YDB ORM for Java (YOJ) is a lightweight ORM for immutable entities. It has native support for YDB and is battle-tested.
Apache License 2.0
13 stars 12 forks source link

Add Snapshot-read-only isolation support #79

Closed jorki02 closed 5 months ago

jorki02 commented 5 months ago

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

nvamelichev commented 5 months ago

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) :hedgehog:

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 :+1:

lavrukov commented 5 months ago

@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 commented 5 months ago

@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 commented 5 months ago

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