[SERVER-40200] Address "query_exec" dependency on "transaction" library. Created: 18/Mar/19 Updated: 29/Oct/23 Resolved: 27/Mar/19 |
|
| Status: | Closed |
| Project: | Core Server |
| Component/s: | None |
| Affects Version/s: | None |
| Fix Version/s: | 4.1.10 |
| Type: | Improvement | Priority: | Major - P3 |
| Reporter: | Justin Seyster | Assignee: | Janna Golden |
| Resolution: | Fixed | Votes: | 0 |
| Labels: | None | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original Estimate: | Not Specified | ||
| Issue Links: |
|
||||||||
| Backwards Compatibility: | Fully Compatible | ||||||||
| Sprint: | Sharding 2019-03-25, Sharding 2019-04-08 | ||||||||
| Participants: | |||||||||
| Description |
|
Planned changes to the TransactionHistoryIterator will require it to call into the query_lib's getExecutorFind() function. However, query_lib already has a dependency on the transaction library, so this call would create a dependency cycle. It looks like this dependency is the result of UpdateStage::assertDocStillBelongsToNode(). Is there some way we can break that dependency so that the transaction library can safely depend on query_lib? |
| Comments |
| Comment by Githook User [ 27/Mar/19 ] |
|
Author: {'email': 'golden.janna@gmail.com', 'name': 'jannaerin', 'username': 'jannaerin'}Message: |
| Comment by Siyuan Zhou [ 19/Mar/19 ] |
|
Agreed. States like whether we are inside a transaction or not should be accessed without linking against the whole transaction library. Currently, the member variables of operation context and its decorations serve this purpose. Maybe in the future we need to go further than that to pull out more states, following how sharding pulled out the sharding API for upper level usage. The implementation of transaction shouldn't be linked anyway though. |
| Comment by Kaloian Manassiev [ 19/Mar/19 ] |
|
Assigning to janna.golden to see whether this dependency on the transactions library could be abstracted. I kind of think though, that being able to access the active transaction's state from within an update is not architecturally wrong. Perhaps the parts of the transactions library, which use DBDirectClient can be pulled out? |
| Comment by Siyuan Zhou [ 18/Mar/19 ] |
|
kaloian.manassiev, this is the issue we discussed the other day and it turned out to be blocking the change stream work. I think this ticket is what we need. Transaction is implemented by writing and reading the metadata in transaction table, so it's natural to have TransactionHistoryIterator to depend on query. Currently, DBDirectClient is used, so the dependency cycle already exists but it's just hidden. The only place where query needs transaction is here. That logic needs to know whether we are in a transaction or retryable write but it doesn't depend on the implementation of transaction. There are perhaps other ways to achieve this. For example, we can check the read concern: snapshot to know whether users intend to run a transaction. Checking the transaction number will tell us whether a session is used. Checking the opCtx->getWriteUnitOfWork() can tell whether the transaction is active or not. Hope we can remove the cycle somehow. |
| Comment by Kaloian Manassiev [ 18/Mar/19 ] |
|
It doesn't seem right that the transactions library should have a dependency on query. Transactions is a very low level concept and if anything, query should have dependency on it. What is the scenario for adding this dependency? |