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

Handle unprocessed deletes without throwing an IllegalArgumentException #187

Merged
merged 3 commits into from
Jun 6, 2024

Conversation

szabado-faire
Copy link
Contributor

I started running into exceptions that looked like the following:

java.lang.IllegalArgumentException: Cannot find a dynamodb table for class software.amazon.awssdk.enhanced.dynamodb.Key
	at app.cash.tempest2.internal.DynamoDbLogicalDb.expectedRawItemType(DynamoDbLogicalDb.kt:390)
	at app.cash.tempest2.internal.DynamoDbLogicalDb.rawItemKey(DynamoDbLogicalDb.kt:376)
	at app.cash.tempest2.internal.DynamoDbLogicalDb.toBatchWriteResponse(DynamoDbLogicalDb.kt:308)
	at app.cash.tempest2.internal.DynamoDbLogicalDb.access$toBatchWriteResponse(DynamoDbLogicalDb.kt:65)
	at app.cash.tempest2.internal.DynamoDbLogicalDb$Sync.batchWrite(DynamoDbLogicalDb.kt:120)
	at app.cash.tempest2.LogicalDb.batchWrite(LogicalDb.kt:125)
	at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103)
	at java.base/java.lang.reflect.Method.invoke(Method.java:580)
	at app.cash.tempest.internal.ProxyFactory$invocationHandler$1.invoke(ProxyFactory.kt:55)
[skipping the private part of the stack trace]

unprocessedDeleteItemsForTable returns Key, but line 308 tries to call rawItemKey() on it and chokes. The net result is that tempest can't handle unprocessed deletes. Removing the unnecessary conversion should resolve it.

I couldn't see a way to verify that this would solve the problem, as there don't seem to be any tests around unprocessedDeletes. I tried a similar approach to my test here, but there's no way to delete more than 16MB. There's a max of 25 items, and each one can be 400KB. If each one is as big as possible, you only hit 10MB. Without mocking the dynamo client, there's no way to trigger a failure here.

The easiest way to test this is probably to expose the toBatchWriteResponse as an internal function and unit test it - there isn't an existing practice of that in this project so I was hesitant to do so. Let me know if you'd like a test added that follows that approach.

@kyeotic
Copy link
Collaborator

kyeotic commented Jun 6, 2024

This change looks like this should be breaking. I definitely want to get a test on this to confirm, especially that the old way breaks. One way to force a failure would be to set an actual throughput on the table and overload it with a large batch. I believe dynamodocker and dynamodb-local both respect non-zero values.

I did some spot checking on internal usage, and all the high volume stuff uses TransactionWrite for delete, which doesn't return unprocessed keys. Its possible we just haven't exercised this sufficiently, but I would still be surprised if this just never worked.

@szabado-faire
Copy link
Contributor Author

szabado-faire commented Jun 6, 2024

Unfortunately local dynamo does not respect the limits (docs):

Provisioned throughput settings are ignored in downloadable DynamoDB, even though the CreateTable operation requires them.

@szabado-faire
Copy link
Contributor Author

@kyeotic Had a moment of brilliance - I realized that testing this is actually super easy. Added a test for it; the test fails if you revert my change

Copy link
Collaborator

@kyeotic kyeotic 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 would prefer a real test, since this depends very critically on the behavior of this fake, but barring a way to get dynamo-local to fail I suspect this is as good as we can get.

@kyeotic kyeotic merged commit 397e6af into cashapp:main Jun 6, 2024
2 checks passed
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