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

Introduce TransactionEndTask. Implements #531 #532

Merged
merged 1 commit into from
Dec 4, 2017

Conversation

dmfs
Copy link
Owner

@dmfs dmfs commented Dec 2, 2017

This commit introduces a TransactionEndTask which is run right before a transaction is ended.
It also adds a simple implementation of TransactionEndTask which performs the call to setTransactionSuccessful() and runs it as the very last of these tasks.

This commit introduces a TransactionEndTask which is run right before a transaction is ended.
It also adds a simple implementation if TransactionEndTask which performs the call to setTransactionSuccessful() and runs it as the very last of these tasks.
@dmfs dmfs requested a review from lemonboston December 2, 2017 23:44
@lemonboston
Copy link
Contributor

The changes look correct to me.

But looking into these transaction codes and seeing the javadoc for beginTransaction about the suggested usage:

db.beginTransaction();
try {
   ...
   db.setTransactionSuccessful();
} finally {
   db.endTransaction();
}

this clearly looks like a template, which is in fact repeated in SQLiteContentProvider. I suppose it could be extracted, and having a type that is executed in the middle, taking SQLiteDatabase as parameter.
The end transaction could be of the same type.
Not sure if this is worth attending now or not, so I just set my review to approved.

@lemonboston
Copy link
Contributor

lemonboston commented Dec 4, 2017

Something like this (quickly written down, not sure if it's correct):

interface SqlLiteTransactionContent<T>
{
       T execute(SqlLiteDatabase db);
}
// T could be Uri or Integer(count)

interface SqlLiteTransaction
{
       T execute(SqlLiteTransactionContent<T> content) 
}
// (edit: added T as return value)

// end transaction could be SqlLiteTransactionContent<T> decorator, returning the delegate result

// usage:
new BasicSqlLiteTransaction(db).execute(new Ended(endContents, new Insert(..)));
// the end transactions could also be added by secondary SqlLiteTransaction ctors
// the template would be implemented in BasicSqlLiteTransaction (it could even be SqlLiteTransactionContent decorator, although not sure if that's a good idea)

@dmfs
Copy link
Owner Author

dmfs commented Dec 4, 2017

Actually I already have something like that in a separate branch. It's slightly more complicated than that because as you can see in the code it depends on a ThreadLocal variable whether the current transaction is marked successful right away or not. I'm currently in the process of refactoring this class (which originated in Android's Calendar Provider).

The major issue is that applyBatch receives ContentProviderOperations which call the provider but need to be executed in a single transaction, hence we can't just commit each transaction in insert, etc.

Anyway, I want to do this step by step. The call to setTransactionSuccessful will certainly be moved somewhere else during that process.

@lemonboston
Copy link
Contributor

I see. This redesign and refactor certainly looks like something that may result in a big changeset.

@dmfs dmfs merged commit 85a0f81 into master Dec 4, 2017
@dmfs dmfs deleted the stories/531-transactionendtasks branch December 4, 2017 20:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants