IGNITE-28727 Create internal API method to lock cache entry with specific version#13264
IGNITE-28727 Create internal API method to lock cache entry with specific version#13264vldpyatkov wants to merge 3 commits into
Conversation
vldpyatkov
force-pushed
the
ignite-28727
branch
3 times, most recently
from
2b29e34 to
63cf3ce
Compare
…metric interceptor keep binary flag.
tkalkirill
changed the title
tkalkirill
left a comment
tkalkirill
left a comment
There was a problem hiding this comment.
A few additional general comments:
- Please add tests where a lock is acquired via the new API in one transaction and an attempt is made to acquire it via the new API in another; cover both distinct keys and the same key. It would be great to test this with both single-threaded and multi-threaded scenarios.
- I didn't see a test checking what happens if the new API is called outside of a pessimistic transaction.
- I didn't see a test checking the behavior regarding lock acquisition after the transaction has completed.
4.waitTimeoutExpiresFirstshould be moved to a shared location so there is only one instance of it, accompanied by a clear comment.
| * @return Future that resolves to {@code true} if the lock was acquired and the versions matched, or to | ||
| * {@code false} otherwise. | ||
| */ | ||
| public IgniteInternalFuture<Boolean> lockTxEntryAsync(CacheEntry<K, V> entry, long waitTimeout); |
There was a problem hiding this comment.
What about errors? If it fails, will the method throw an error or will the future complete with an error? Please reflect this in the documentation.
There was a problem hiding this comment.
Do you mean a concrete scenario or just in general? I am sure the method can send similar exceptions like other transactional operations (for example, putAllAsync/putAsync) but there are no notes.
I can underline NPE in case there is a null passed as a entrty/collection because it's done right in the method methodA.notNull.
Would it be enough?
| * @return Future that resolves to {@code true} if all locks were acquired and all versions matched, or to | ||
| * {@code false} otherwise. | ||
| */ | ||
| public IgniteInternalFuture<Boolean> lockTxEntriesAsync(Collection<CacheEntry<K, V>> entries, long waitTimeout); |
There was a problem hiding this comment.
What about errors? If it fails, will the method throw an error or will the future complete with an error? Please reflect this in the documentation.
There was a problem hiding this comment.
The same as above. We do not describe exceptions with the future that can fail in the other methods. So I do not think it is needed here.
| IgniteTxEntry txEntry = internalCache.tx().entry(txKey); | ||
|
|
||
| assertNotNull(txEntry); | ||
| assertEquals(skipStore, txEntry.skipStore()); |
There was a problem hiding this comment.
Could you explain what these flags mean and why they need to be checked?
I tried looking at the documentation for them, but I still didn't quite understand.
There was a problem hiding this comment.
This flag is used to enlist phase in the other operation (put/get e.t.c.). I checked it because in the new method I did not use the old variants of transaction enlist.
| @Override protected GridCacheReturn postLock(GridCacheReturn ret) throws IgniteCheckedException { | ||
| assert fut.error() == null; | ||
|
|
||
| boolean success = Boolean.TRUE.equals(fut.get()); |
There was a problem hiding this comment.
Same about fut.get() as above.
There was a problem hiding this comment.
The same; it is a code that is executed after the nested future is complete.
| new IgniteCheckedException("Failed to acquire transactional lock in optimistic transaction.")); | ||
|
|
||
| // Wait for previous per-transaction async operations to finish. | ||
| tx.txState().awaitLastFuture(); |
There was a problem hiding this comment.
I did't understand the reason for this wait.
There was a problem hiding this comment.
We are doing the same in the transaction methods (put/get e.t.c), we just do it in the helpers (syncOp, asyncOp).
The real meaning is waiting for the previous transaction operation completion.
|
About the text comment above:
|
Possible compatibility issues. Please, check rolling upgrade casesThis PR modifies protected classes (with Order annotation). Affected files:
|
2 participants
https://issues.apache.org/jira/browse/IGNITE-28727