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

Transactions simplified to use distributed transaction manager only. #4568

Merged
merged 1 commit into from
May 11, 2018

Conversation

jason-bragg
Copy link
Contributor

@jason-bragg jason-bragg commented May 8, 2018

Removed singleton transaction manager and abstractions related to that pattern.
Still work in progress, as it should be rebased on Sebastian's transaction storage work, once that goes in.

Removed dynamoDb transaction work because there was nothing left once log storage was removed. Related assemblies and tests are still under source control and can be brought back if/when transactional storage impl is introduced.

@jsteinich
Copy link
Contributor

This seems like a rather large change and is a bit hard to follow through diffs. Admittedly I haven't dug into the Distributed TM in much detail, but perhaps a short summary of it would help.

Looking through, the changes to ITransactionalState seem a bit inconvenient. I understand why they would be needed, but it adds another layer (at least of indents) to what could be simple methods.

I also see #4566 and it is a bit unclear (without spending more time reading it) what exactly is getting stored. Is is just information about the transactions or is it actually the "grain" state? If it is actually the grain state that is possibly a blocker for our use. If it is just transaction information we'll need a DynamoDB implementation. I'm happy to help with that once I get a chance to read through everything more.

Copy link
Contributor

@sebastianburckhardt sebastianburckhardt left a comment

Choose a reason for hiding this comment

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

Seems all very straightforward; some changes overlap with #4566. I haven't yet looked into test results but it is probably better to do that after we merge.

services.TryAddSingleton<Factory<ITransactionAgent>>(sp => () => sp.GetRequiredService<ITransactionAgent>());
services.TryAddSingleton<ITransactionManagerService, DisabledTransactionManagerService>();
//services.TryAddSingleton<ITransactionAgent, DisabledTransactionAgent>();
Copy link
Contributor

Choose a reason for hiding this comment

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

is this comment still needed? Seems that the DisabledTransactionAgent is gone.

Task<CommitTransactionsResponse> CommitTransactions(List<TransactionInfo> transactions, HashSet<long> queries);
Task AbortTransaction(long transactionId, OrleansTransactionAbortedException reason);
}

[Serializable]
public struct CommitResult
Copy link
Contributor

Choose a reason for hiding this comment

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

Are the structs in this file still used somewhere? I can't find any occurrences.

@jason-bragg jason-bragg force-pushed the KillSingletonTransactionManager branch from e3218e6 to e4c16ed Compare May 9, 2018 21:27
@jason-bragg jason-bragg changed the title wip - Transactions simplified to use distributed transaction manager only. Transactions simplified to use distributed transaction manager only. May 9, 2018
@jason-bragg
Copy link
Contributor Author

@dotnet-bot test this please

@jason-bragg
Copy link
Contributor Author

@jsteinich At a high level, the distributed TM pattern picks a grain involved in the transaction to act as the TM for that transaction, We don't have any design documentation for this (as far as I know) as it was developed in research.

The distributed TM logic was check in previously in "draft of distributed TM implementation #3820". May be easyer to look at that PR when familiarizing yourself. This PR mostly just pulls out the singleton TM and make the distributed tech the only one we support.

@xiazen
Copy link
Contributor

xiazen commented May 10, 2018

I see 369 bvt failing, while majority of them isn't tx related. WHat's going on? are they pass locally?

@jason-bragg
Copy link
Contributor Author

No, It's a bug. Fixing. Transaction agent is only added in transaction tests, but requested by all. Adding default agent.

@jason-bragg jason-bragg force-pushed the KillSingletonTransactionManager branch from 67b5ba8 to ef62122 Compare May 10, 2018 21:28
@jason-bragg
Copy link
Contributor Author

@dotnet-bot test functional please

@xiazen xiazen merged commit 8d1c619 into dotnet:master May 11, 2018
@github-actions github-actions bot locked and limited conversation to collaborators Dec 5, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants