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

Cleanup pass on Transaction abstractions and public surface. #4628

Merged
merged 2 commits into from
May 31, 2018

Conversation

jason-bragg
Copy link
Contributor

  • Removed leftover code from singleton TM.
  • Moves agent abtraction from core to runtime abstractions
  • Cleaned up agent initialization and usage pattern
  • Reduced public surface of transactional state abstraction
  • Moved transaction info out of abstractions
  • Removed old unsused logging errorcodes.
  • Moved serialization generation assembly statements to files that defined the serializable classes.

/// </summary>
internal enum OrleansTransactionsErrorCode
{
/// <summary>
/// Start of orlean servicebus errocodes
/// Start of orlean transactions errocodes
Copy link
Member

Choose a reason for hiding this comment

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

Orleans
error codes

{
logger.Error((int)errorCode, message, exception);
}
// TODO - jbragg - add error codes for transaction errors - User Story 62
Copy link
Member

Choose a reason for hiding this comment

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

IMO we should add a link to the story in VSO - it's easy to forget where User Story 62 lives

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tasking is a mess right now, will remove reference to specific task as odds of link or task remaining the same by the time we get to fixing this is fairly weak.

using Orleans.Concurrency;

[assembly: GenerateSerializer(typeof(Orleans.Transactions.Abstractions.PendingTransactionState<>))]
[assembly: GenerateSerializer(typeof(Orleans.Transactions.Abstractions.TransactionalStorageLoadResponse<>))]
Copy link
Member

Choose a reason for hiding this comment

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

I prefer assembly-level attributes live in a file called AssemblyInfo.cs which only contains assembly-level metadata

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you provide your reasoning?

I've moved to this pattern due to maintainability. Having this declared in the same file as the type makes the serialization expectation more explicit to the maintainers. Until we resolve our backwards compatibility issues, this sort of awareness is very important.

Copy link
Member

@ReubenBond ReubenBond left a comment

Choose a reason for hiding this comment

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

added some comments, but nothing blocking

- Removed leftover code from singleton TM.
- Moves agent abtraction from core to runtime abstractions
- Cleaned up agent initialization and usage pattern
- Reduced public surface of transactional state abstraction
- Moved transaction info out of abstractions
- Removed old unsused logging errorcodes.
- Moved serialization generation assembly statements to files that defined the serializable classes.
- Cleand up comments and typos
@jason-bragg jason-bragg force-pushed the TransactionsCleanup branch from cf4163f to 0dbff8f Compare May 29, 2018 21:05
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.

LGTM.
I saw there is a TODO for the transaction error codes, one could consider doing that as part of this PR.

@xiazen xiazen merged commit 65431e2 into dotnet:master May 31, 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants