Introducing DynamoDB interception via SDK client class replacement#1495
Introducing DynamoDB interception via SDK client class replacement#1495aschenzle wants to merge 8 commits intoWebFuzzing:masterfrom
Conversation
…ing transactional operations and SQL like queries.
…ing transactional operations and SQL like queries.
…ing transactional operations and SQL like queries.
|
|
||
| protected static Object handle(ThirdPartyMethodReplacementClass singleton, Object client, String id, Object request, String operationName) { | ||
| long start = System.currentTimeMillis(); | ||
| boolean isAsync = client.getClass().getName().contains("Async"); |
There was a problem hiding this comment.
Why don't we habe a handleAsync() and handleSync() and we call each method replacement differently?
There was a problem hiding this comment.
was trying to optimize for code reuse as there will be 8 lines of repeated code if we split the methods. I'm ok changing if that fits better with project conventions. Please confirm you want this change and I'll do it.
| return Collections.singletonList(tableName); | ||
| } | ||
| } catch (NoSuchMethodException ignored) { | ||
| // no-op |
There was a problem hiding this comment.
Missing a really important comment. The following comment will replace that no-op: "// Ignored in the case we are calling for a batch request, it will be handled below in the next section" Let me know if that's clear enough. Or you also want to split the logic for singleTable calls vs batch requests.
| Collections.sort(tableNames); | ||
| return tableNames; | ||
| } catch (NoSuchMethodException | IllegalAccessException | InvocationTargetException e) { | ||
| return Collections.emptyList(); |
There was a problem hiding this comment.
Why is is defaulted to empty?
There was a problem hiding this comment.
Seemed like the best option, as we don't want the interception to throw exceptions and retuning null requires handling it. We are just getting less metadata about the interception for this particular request if for some weird reason it fails (Only case I can think this failing would be a dependency problem with another version of the SDK breaking API compatibility, not expected from AWS).
There was a problem hiding this comment.
Why don't you encapsulate this and throw a RuntimeException?
| <groupId>io.lettuce</groupId> | ||
| <artifactId>lettuce-core</artifactId> | ||
| <version>${io.lettuce.core.version}</version> | ||
| <!-- Need to exclude Netty from this package to avoid a clash of versions with AWS SDK --> |
There was a problem hiding this comment.
Could you improve the comment why this exclusion is needed?
There was a problem hiding this comment.
How about "Need to exclude Netty from this package to avoid a conflict of versions with the Netty dependency introduced by AWS SDK, the AWS SDK version of Netty is newer and all e2e tests pass with it."?
| <dependency> | ||
| <groupId>software.amazon.awssdk</groupId> | ||
| <artifactId>netty-nio-client</artifactId> | ||
| <version>${software.amazon.awssdk.dynamodb.version}</version> |
There was a problem hiding this comment.
Shouldn't the scope of this dependency be also test?
2 participants
Intercepting main operations (Query, Scan, GetItem, BatchGetItems, PutItem, UpdateItem and Delete Item) for both sync and async DynamoDB SDK clients. The interception happens at the SDK level and I've verified it works via an integration test using Docker. There was a dependency conflict while bringing the AWS SDK through Maven, Netty versions would get mixed up for Redis end to end tests so I've made the decision to exclude Netty from Lettuce package and use the Netty version packaged on AWS SDK. All e2e tests are passing.
As this is my first PR I'm looking for exhaustive feedback to quickly learn any conventions I've might missed.