-
Notifications
You must be signed in to change notification settings - Fork 135
IGNITE-21291 Scan cursors do not close when an RO transaction is finalized #3219
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
Conversation
| /** | ||
| * Retrieves a collection of transaction UUIDs. | ||
| * | ||
| * @return a collection of transaction UUIDs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| * @return a collection of transaction UUIDs | |
| * @return A collection of transaction UUIDs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| /** | ||
| * Keeps track of all closed RO transactions. | ||
| */ | ||
| public class ClosedTransactionTracker { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In fact, it works only with RO transactions. Maybe rename it to ClosedReadOnlyTransactionTracker?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed
| */ | ||
| public class ClosedTransactionTracker { | ||
|
|
||
| private static final int maxClosedTransactionsInBatch = 10_000; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be upper case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| /** | ||
| * Topology service. | ||
| */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| /** | |
| * Topology service. | |
| */ | |
| /** Topology service. */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| UUID txId = TestTransactionIds.TRANSACTION_ID_GENERATOR.transactionIdFor(readTimestamp); | ||
|
|
||
| var tx = new ReadOnlyTransactionImpl(txManager, new HybridTimestampTracker(), txId, "localId", readTimestamp); | ||
| var tx = new ReadOnlyTransactionImpl( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about adding additional tests checking that RO transaction's cursors are closed after its finish?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a test to cover that
| private void processTxCleanup(CloseCursorsBatchMessage closeCursorsMessage) { | ||
| closeCursorsMessage.transactions().forEach(resourcesRegistry::close); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here we can process potentially huge batch of transactions (tracker allows max size of 10k), I an not sure it's a good idea to do this in network thread. What do you think about doing this in cleanup thread within resource cleanup manager in background?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| * Close transaction cursors message. | ||
| */ | ||
| @Transferable(TxMessageGroup.TX_CLOSE_CURSORS) | ||
| public interface CloseCursorsBatchMessage extends TimestampAware { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about FinishedTransactionsBatchMessage?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed
| int cpus = Runtime.getRuntime().availableProcessors(); | ||
|
|
||
| cleanupExecutor = new ThreadPoolExecutor( | ||
| writeIntentSwitchPool = new ThreadPoolExecutor( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It think it's worthy to rename as well
TxManagerImpl#executeCleanupAsyncIgniteThreadFactory.create(nodeName, "tx-async-cleanup", LOG, STORAGE_READ, STORAGE_WRITE)- thread name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed
https://issues.apache.org/jira/browse/IGNITE-21291