Adding Transactions #92
Conversation
There was a problem hiding this comment.
Nice work! You have shown that the usage patterns for TX that I assumed when creating the ticket are in fact different to the normal auto-batching use case for Gets/Puts/Deletes and I agree with your approach.
I have a suggestion for capturing the difference in functionality with a different trait hierarchy (not sure if it'll work though)
It may be worth running the whole thing by John and getting his feedback.
Great work!
need to do batchGets/batchWrites as well as figure out condition checks
| @@ -200,6 +200,27 @@ object LiveSpec extends DefaultRunnableSpec { | |||
| withDefaultTable { tableName => | |||
| getItem(tableName, PrimaryKey(id -> "nowhere", number -> 1000)).execute.map(item => assert(item)(isNone)) | |||
| } | |||
| }, | |||
| testM("batch get item") { | |||
There was a problem hiding this comment.
how about
| testM("batch get item") { | |
| testM("batch get item with a transaction") { |
? or move it to
suite("transactions") below?
There was a problem hiding this comment.
This was actually supposed to just be a batch get instead of a batch get transaction. I noticed we didn't have an integration test for the batch gets. I'll remove the .transaction from it.
| testM("batch get item") { | ||
| withDefaultTable { tableName => | ||
| val getItems = BatchGetItem().addAll( | ||
| GetItem(TableName(tableName), Item(id -> first, number -> 7)), |
There was a problem hiding this comment.
ATM we have to mentally track that Item(id -> first, number -> 7) is the PK of avi3Item.
How about creating a utility function for the whole suite, something like def pk(item: Item): PrimaryKey = ??? as I think id and number are the primary keys for the test data used throughout the suite. This line would then become:
| GetItem(TableName(tableName), Item(id -> first, number -> 7)), | |
| GetItem(TableName(tableName), pk(avi3Item)), |
so we end up a direct a correlation between the item and its primary key.
There was a problem hiding this comment.
Have a weird implementation that has a dummy PK for its default, let me know what you think.
There was a problem hiding this comment.
I think its a big improvement on readability. WRT the implementation I was thinking of a direct transformation of the input eg something like
private def pk(item: Item): PrimaryKey =
(item.map.get("id"), item.map.get("num")) match {
case (Some(id), Some(num)) => PrimaryKey("id" -> id, "num" -> num)
case _ => throw new IllegalStateException(s"Both id and num need to present in item $item")
}There was a problem hiding this comment.
I'll update this tonight before merging
| ) | ||
|
|
||
| for { | ||
| _ <- conditionCheck.zip(putItem).transaction.execute |
There was a problem hiding this comment.
nice semantics - I like it!
| val conditionCheck = ConditionCheck( | ||
| primaryKey = PrimaryKey(id -> first, number -> 7), | ||
| tableName = TableName(tableName), | ||
| conditionExpression = ConditionExpression.Equals( |
There was a problem hiding this comment.
how about extracting this to a val called something like conditionAlwaysTrue = ???
dynamodb/src/main/scala/zio/dynamodb/DynamoDBExecutorImpl.scala
Outdated
Show resolved
Hide resolved
| .foldLeft(headConstructor: Either[Throwable, TransactionType]) { | ||
| case (acc, constructor) => | ||
| acc match { | ||
| case l @ Left(_) => l // Should also continue collecting other failures |
There was a problem hiding this comment.
how about creating an issue for collecting failures so we can track this (collecting multiple failures)? I think zio-aws for ZIO 2.0 is bringing in zio-prelude as a transitive dependency anyway so maybe we should consider zio-prelude Validation internally (I have used it in production and the ergonomics are really nice)
dynamodb/src/main/scala/zio/dynamodb/DynamoDBExecutorImpl.scala
Outdated
Show resolved
Hide resolved
| billingMode = BillingMode.PayPerRequest | ||
| ).transaction.execute | ||
| )(equalTo(())) | ||
| } @@ failing, |
There was a problem hiding this comment.
again it would be nice to zoom in on the specific exception type rather than use catch all failing (and perhaps some small sub-string of the message we are expecting) eg something like
def assertDynamoDbException(substring: String) =
isSubtype[DynamoDbException](hasMessage(containsString(substring)))
val suiteExperiment = suite("experiment")(testM("foo") {
val program = ZIO.effect(throw DynamoDbException.builder.message("Transaction gone BOOOOOOM!").build)
assertM(program.run)(fails(assertDynamoDbException("BOOOOOOM")))
})There was a problem hiding this comment.
Updated all of these failing to be more specific as well
|
PS from now on we will also have to create an issue to copy over PR changes once merged to the ZIO 2 branch |
|
I created two issues, one for collecting all of the invalid transaction items and another for merging the transaction changes into the zio2 branch. |
Adding transaction functionality. A user can now build a DynamoDBQuery and call
.transactionon it to have it be executed as a single transaction. Currently if there are problems with the transaction, like mixing Get and Write actions, the user will get a failing ZIO effect. There is also asafeTransactionmethod that returns anEither[Throwable, DynamoDBQuery[A]]for type safe transactions. I'm happy to change that method name if someone has a different suggestion.