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

Add DynamoDB Enhanced Client for DynamoDB Annotations #255

Merged
merged 1 commit into from Sep 23, 2022

Conversation

hamburml
Copy link
Contributor

@@ -49,6 +49,25 @@
</exclusion>
</exclusions>
</dependency>
<dependency>
<groupId>software.amazon.awssdk</groupId>
<artifactId>dynamodb-enhanced</artifactId>
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if we should add it here or if it should be a separate artifact. Depends on if people will massively move to it I suppose or if they will want to use the low level client without bringing this one?

Also, you know I was doing to ask for that :) : we need a test, especially one that also runs in native as we need to make sure it works with native (and native might require some additional work at the extension level).

@scrocquesel do you have an opinion on this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a test for async and sync. Unfortunately it is not working, I always get software.amazon.awssdk.core.exception.SdkClientException: Unable to instantiate request handler chain for client. Listed request handler ('software.amazon.awssdk.enhanced.dynamodb.internal.ApplyUserAgentInterceptor') does not implement the interface software.amazon.awssdk.core.interceptor.ExecutionInterceptor API. Maybe the Enhanced Client simply does not work with execution interceptor.

Any advice is highly appreciated

Copy link
Member

Choose a reason for hiding this comment

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

Not sure if we should add it here or if it should be a separate artifact. Depends on if people will massively move to it I suppose or if they will want to use the low level client without bringing this one?

Also, you know I was doing to ask for that :) : we need a test, especially one that also runs in native as we need to make sure it works with native (and native might require some additional work at the extension level).

@scrocquesel do you have an opinion on this?

TBH, i'm not doing dev with aws, so I do not have strong opinion about having this deps in the extension. But as you said, if added, native mode should be thorougly tested and community should be prepared to give enough support if this deps often change. Maybe as a separate extension ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a test for async and sync. Unfortunately it is not working, I always get software.amazon.awssdk.core.exception.SdkClientException: Unable to instantiate request handler chain for client. Listed request handler ('software.amazon.awssdk.enhanced.dynamodb.internal.ApplyUserAgentInterceptor') does not implement the interface software.amazon.awssdk.core.interceptor.ExecutionInterceptor API. Maybe the Enhanced Client simply does not work with execution interceptor.

Any advice is highly appreciated

FYI - If I do not set quarkus.class-loading.reloadable-artifacts=software.amazon.awssdk:dynamodb-enhanced I get the NoClassDefFoundError which is stated in the aws sdk issue.

Maybe as a separate extension?

If I understood it correctly the dynamodb enhanced client depends on the dynamodb client. https://github.com/aws/aws-sdk-java-v2/blob/master/services-custom/dynamodb-enhanced/pom.xml#L130 I have no experience with Quarkus extensions, but if they depend on each other doesn't it make more sense to do this in one extension?

LOG.info("Testing Async Dynamodb client with table: " + ASYNC_TABLE);
String partitionKeyAsString = UUID.randomUUID().toString();

DynamoDbEnhancedAsyncClient enhancedAsyncClient = DynamoDbEnhancedAsyncClient.builder()
Copy link
Member

Choose a reason for hiding this comment

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

I guess, the instance should be provided by the extension

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you mean? According to the aws v2 sdk a DynamoDBEnhancedClient can use an underlying DynamoDBClient. Isn't it safer to follow the sdk way? Could you elaborate what you mean?

Copy link
Member

Choose a reason for hiding this comment

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

I mean a producer so you can just inject the enhanced version like the other one. I guess the user only need one version at a time or is it design so that you need both version to cover all sdk api ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think you need both if you want to cover all sdk (low-level, high-level). I also do not know if the enhanced client replaces the other client. To be honest, I don't think so. They are even in seperate locations (https://github.com/aws/aws-sdk-java-v2/tree/master/services/dynamodb vs https://github.com/aws/aws-sdk-java-v2/tree/master/services-custom/dynamodb-enhanced).

@hamburml
Copy link
Contributor Author

Yesterday I also found this extension https://github.com/Nithanim/quarkus-dynamodb-enhanced. In the readme this issue aws/aws-sdk-java-v2#2604 is mentioned. I think to fix this myself exceeds my current capabilities in quarkus (classloader magic) and aws.

@Nithanim
Copy link

@hamburml Hello! Thank you for contacting me! I am very sorry but I currently do not have the mental capacity to help you with your issues trying to integrate the enhanced client into the Quarkus extension because I am quite burnt out by my dayjob. Also I don't want to deal with tests and autoconfig additionally, which was pretty much why I just made my own Quarkus extension and left it at that.

BUT I am gladly willing to give you any insight I have around what my extension does and how it works. In my repository you should find a lot of documentation about the issues and what I did to fix them but if something is still not clear just message me somewhere (here, reddit, telegram, ...).

Basically, Quarkus has a special classloader thingy going for tests which breaks aws-sdk code which can't be fixed (from my point of view) in Java 8 (but the sdk is built against Java 8). And the BeanTableSchema does not work with Graal native-image because of "reflection" (not really but for simplicity here) which my extension also circumvents.

Otherwise, the dynamodb enhanced client should be compatible with Graal native-image. At least for all features we used at my previous work. Coincidentally, today they messaged me and told me that they upgraded Quarkus to the newest version, including my extension. It happily helps serving and booking holiday homes with multiple partners without issue every day.

By the way, you have full permission to copy paste stuff from my repository if you need it. If you manage to integrate it into the official extension I would be very happy, because I switched jobs and migrated to Azure. So if you managed to lift the burden off me that would be great!

Regarding the integration of the enhanced client into the existing dynamodb extension: I would also suggest to create a new extension /dynamodb-enhanced right beside the existing /dynamodb to keep them separated. Makes sense both feature-wise and test-wise. So if one does not want the enhanced client, it is not included.

@Nithanim
Copy link

Regarding the problem with the SdkClientException: I don't know what quarkus.class-loading.reloadable-artifacts does but I think that the rest of the message is misleading you a bit. If the same class is loaded by multiple classloaders, everything about the class looks exactly the same but for the JVM, they are completely different. I suspect that this is exactly the case here as the error message does not convey this important information. Pretty sure you can see it in the debugger when you look at the hashcode or the toString() of the classes.

However, I have not encountered encountered your specific error with ApplyUserAgentInterceptor yet. I only did with user-classes like the database-beans you define. So I am not really sure what the problem is there or even what the class is for. (Or it is in your commit but I haven't looked at your code, sorry.) But I can give you a short explanation what happens in the case I fixed with the database-beans:

The NoClassDefFoundError happens because, well, it is exactly what it states it is. In short, Quarkus loads all dependencies in one classloader and the classes of the current maven module in another. But the aws sdk has a "bug" in there which leads to the point that the sdk tries to search for the user-class (the dynamo db bean or whatever) in its classloader but this is completely wrong because the stuff is in a different classloader. Hence, the NoClassDefFoundError with stuff that is there (in your point of view).

@hamburml
Copy link
Contributor Author

hamburml commented Aug 30, 2022

@Nithanim Thanks for answering and your explanations. Quarkus dropped java 8 support with the release of v 2. So maybe your fixes are already enough to make it work and we do not need to bother with a fix for version 8.

I will try to update the PR and add it next do dynamodb so that in the end we have two artifacts.

@scrocquesel @gsmet If you have time could you cross-read and give your opinion? I don't want to invest too much time in case the fixes used in https://github.com/Nithanim/quarkus-dynamodb-enhanced are not wanted for this extension.

@hamburml
Copy link
Contributor Author

hamburml commented Aug 30, 2022

To be honest, I simply copied quarkus.class-loading.reloadable-artifacts from another comment and it worked on my machine, the NoClassFoundError was gone. But now another error regarding ApplyUserAgentInterceptor appeared during the integration-tests. I think the reloadable-artifacts is not needed when its fixed and is a side effect from loading the classes with another classloader :)

The class can be found here btw https://github.com/aws/aws-sdk-java-v2/blob/master/services-custom/dynamodb-enhanced/src/main/java/software/amazon/awssdk/enhanced/dynamodb/internal/ApplyUserAgentInterceptor.java and your explanations makes totally sense.

@Nithanim
Copy link

@Nithanim Thanks for answering and your explanations.

Absolutely no problem! :) I can gladly help with stuff I found out previously but don't have the capacity for new stuff if that makes any sense.

Quarkus dropped java 8 support with the release of v 2. So maybe your fixes are already enough to make it work and we do not need to bother with a fix for version 8.

Maybe I was not really clear or cut my explanation too short. The fundamental problem is the code of the aws sdk, not Quarkus. Quarkus rather just exposes the problem. However, the aws sdk devs are not to blame either because they simply cannot fix the problem (from my point of view; I tried really hard) as long as they support Java 8. The only two options are that they drop Java 8 support and fix the issue or make a multi-release jar where the Java 8 version has the bug and 9+ (I think) does not.

The first option is pretty much out of reach as long as they support Java 8 for lambdas (and other things). And the second option is a very big pain in the ass to do and nobody is that insane. Also, for Quarkus this is a test issue only since at runtime everything is in the same classloader and works flawlessly. So nobody really cares I guess.

The good coincidence for me was that the classloader issue can be circumvented with the exact same fix that is used for the native-image. I found out about the test-classloader issue after I fixed native-image because I wrote the tests afterwards. So basically the code that replaces the "reflective" access of the user-provided dynamodb bean with a supported "reflective" version by native-image, can be used for the tests. But this is really just a coincidence and I just rolled with it. I think it is a performance hit it you do not native-compile the app. So realistically, you would apply the the patch only for tests and native-image but not for JVM in production.

To be honest, I simply copied quarkus.class-loading.reloadable-artifacts from another comment and it worked on my machine, the NoClassFoundError was gone. But now another error regarding ApplyUserAgentInterceptor appeared during the integration-tests. I think the reloadable-artifacts is not needed when its fixed and is a side effect from loading the classes with another classloader :)

The class can be found here btw https://github.com/aws/aws-sdk-java-v2/blob/master/services-custom/dynamodb-enhanced/src/main/java/software/amazon/awssdk/enhanced/dynamodb/internal/ApplyUserAgentInterceptor.java and your explanations makes totally sense.

It is funny because I don't think that my explanations make any sense in this context 😆. The class is in the original aws sdk, not a user-class. So that should work without any problems because it is all loaded in the same classloader. I am not sure what the problem is and may have to look at that myself.

@hamburml
Copy link
Contributor Author

I started with a new module (I simply copied the files from @Nithanim and created missing files for the new extension). I currently have the issue that multiple extensions registered the feature name amazon-dynamodb-enhanced. This is because of the TestClassloaderProcessor.java and the DynamodbEnhancedProcessor.java. I think I need to mix both classes together but its late and I need to get up tomorrow.... Source is here, if anyone wants to help https://github.com/hamburml/quarkus-amazon-services/tree/feature/dynamodb-enhanced-module

@Nithanim
Copy link

Nithanim commented Aug 31, 2022

@hamburml I took the time to look at your

2022-08-31 21:06:27,223 INFO  [io.qua.it.ama.dyn.DynamoDBEnhancedResource] (executor-thread-0) Testing Blocking Dynamodb client with table: blocking
2022-08-31 21:07:48,368 ERROR [io.qua.ver.htt.run.QuarkusErrorHandler] (executor-thread-0) HTTP Request to /test/dynamodbenhanced/blocking failed, error id: 0103285d-26a2-41cf-b870-00b025254e59-1: java.lang.NoClassDefFoundError: io/quarkus/it/amazon/dynamodb/DynamoDBExampleTableEntry
	at software.amazon.awssdk.enhanced.dynamodb.internal.mapper.ResolvedImmutableAttribute.lambda$create$0(ResolvedImmutableAttribute.java:54)
	at software.amazon.awssdk.enhanced.dynamodb.mapper.StaticImmutableTableSchema.lambda$itemToMap$5(StaticImmutableTableSchema.java:507)
	at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
	at java.base/java.util.Collections$UnmodifiableCollection.forEach(Collections.java:1092)

On first glance it is exactly the problem I solved.
Looking at my solution again, I only did fix the problem for BeanTableSchema but not others. Fixing others is "left as an exercise for the reader". 😉

The following paragraphs give insight into the problem. It is a bit long and if not interested, there is nothing else below.

What is hidden is that the problem is the LambdaToMethodBridgeBuilder.
You can see that if you set a breakpoint at ResolvedImmutableAttribute.java:54 and examine immutableAttribute.getter(),
In the LambdaToMethodBridgeBuilder#build the lambdas are generated at runtime to improve performance accessing the data in the user-provided beans.

The funny thing is, that the code there completes without issues, so a getter/setter/constructor-lambda is returned.
This is the one you currently have in immutableAttribute.getter() at your breakpoint.
As soon as you want to execute the method/lambda some magic happens (I think the real initialization is deferred after creation and is initialized NOW) and then it tries to resolve the class.

The problem lies in the previously mentioned LambdaToMethodBridgeBuilder#build which has the LOOKUPs wrong. It uses its classloader to lookup the user-bean which is no problem normally.
But in our case is is very important to use the correct classloader (the one where the user-bean is loaded).

I tried to fix that but as far as I can tell you there is no way to tell it to do that, except some advances for the LOOKUP that were added after Java 8.

EDIT: Forgot the solution. Replacing the methods that call into the LambdaToMethodBridgeBuilder and replace it with other magic. Oh and for native image, the lambda factory is the problem too because it effectively created classes (the lambdas) at runtime which is not possible. So replacing that with reflection it works.

@hamburml
Copy link
Contributor Author

hamburml commented Sep 1, 2022

I tried to integrate it as far as I could. The results can be found in https://github.com/hamburml/quarkus-amazon-services/tree/feature/dynamodb-enhanced-module. I can not build it, this is the error Failed to load steps from class io.quarkus.amazon.dynamodb.enhanced.deployment.DynamodbEnhancedProcessor. I did not start with the work on supporting @DynamoDbBean. It also looks like that they changed the architecture with the enhanced client. It uses a new Builder and does not implement the AwsBuilder (https://github.com/aws/aws-sdk-java-v2/blob/master/services-custom/dynamodb-enhanced/src/main/java/software/amazon/awssdk/enhanced/dynamodb/DynamoDbEnhancedClient.java#L450) which also means that the code in the common infrastructure does not work well with that new builder.

To be honest, I need to check if this is really worth it or if I simply use the low level client...

@Nithanim
Copy link

Nithanim commented Sep 1, 2022

To be honest, I need to check if this is really worth it or if I simply use the low level client...

Just chill for a bit. Try to not cram everything at once in there. Focus on the base first, like make the fix and make it compilable. Then either make a small app with it and test it as JVM and native-image (like I have in my repo under "test") or make integration tests to verify that it at least runs as JVM (or even native image if that is possible, don't know).
And then, you can build on that by adding support functionality by adding the autoconfig and automatic beans.

I can not build it, this is the error Failed to load steps from class io.quarkus.amazon.dynamodb.enhanced.deployment.DynamodbEnhancedProcessor.

Look at the Caused by:, Caused by:, Caused by:, ...

...
Caused by: java.lang.IllegalArgumentException: Run time configuration cannot be consumed here unless the method is a @Recorder at io.quarkus.amazon.dynamodb.enhanced.runtime.DynamodbEnhancedConfig buildTimeConfig of void io.quarkus.amazon.dynamodb.enhanced.deployment.DynamodbEnhancedProcessor.maybeApplyClassTransformation(io.quarkus.amazon.dynamodb.enhanced.runtime.DynamodbEnhancedConfig,io.quarkus.deployment.annotations.BuildProducer,io.quarkus.deployment.builditem.LaunchModeBuildItem) of class io.quarkus.amazon.dynamodb.enhanced.deployment.DynamodbEnhancedProcessor
	at io.quarkus.deployment.ExtensionLoader.reportError(ExtensionLoader.java:1056)
	at io.quarkus.deployment.ExtensionLoader.loadStepsFromClass(ExtensionLoader.java:718)
	at io.quarkus.deployment.ExtensionLoader.loadStepsFrom(ExtensionLoader.java:212)

The class is in deployment but you are trying to access runtime config properties. If you need more help, you can always ask me, but I haven't done a whole lot with Config and Recorders yet. Only build items.

I did not start with the work on supporting @DynamoDbBean.

Yes, leave unnecessary features aside for now. Make the base work first.

To be honest, I need to check if this is really worth it or if I simply use the low level client...

The thing I found and fixed was "easy". But it takes effort to get it to a releasable state and then actually release it. And then there is the real effort to make it shiny an integrate it as "official" extension.

You chose last but that is where the real reward is. My thingy works, but you have to find it and then trust me. An official extension is easily accessible since it is listed on the website and is in an well-known namespace. And there is a lot more hope for support than from me, a random stranger.

@hamburml
Copy link
Contributor Author

hamburml commented Sep 4, 2022

I think with the current structure of how this repo should be extended with new modules this will not work.

For example the AmazonClientRecorder::configure in quarkus-amazon-common wants a clientBuilder which extends from AwsClientBuilder - The builder from EnhancedDynamoDbClient does not extends from that base class. The EnhancedClients simply does not follow the structure all the other clients (s3, dynamodb, sns, sqs, ...) follow which als mean that I can not do it like all the others module did. @gsmet What is your opinion on that?

My other idea was to simply copy the dynamodb module with the difference that the DynamodbEnhancedClientProducer only Produces DynamoDbEnhancedClient and DynamoDbEnhancedAsyncClient. I tried it but I got an error that the synthetic bean is already registered/added (should be the case because the createSyncBuilder and createAsyncBuilder signatures are the same in the dynamodb and dynamodb-enhanced module...). So I removed the dynamodb module and voila - it kinda worked, I was also able to run it locally and store data into a dynamodb - strangely i wasnt able to load an item, I always got an error that NPE because payload is null - during debugging I saw the table content... I just pushed it.

Thats it for now...

@Nithanim I do not fully understand the LambdaToMethodBridgeBuilder part. But I think I can ignore that for now and try to simply get the module to work... Maybe we should discuss this issue somewhere else (maybe in an github issue or via mail...).

@Nithanim
Copy link

Nithanim commented Sep 4, 2022

@Nithanim I do not fully understand the LambdaToMethodBridgeBuilder part. But I think I can ignore that for now and try to simply get the module to work...

Yes, you can ignore that for now. I just tried to explain what the exact Problem is and what I did to fix it. It is more for the case that if something does not work or changes down the road in the SDK there is at least a bit of a summary. Basically I tell it to replace specific Methods of specific Classes, so if they are changed in any way, things break.

Maybe we should discuss this issue somewhere else (maybe in an github issue or via mail...).

Discussions about the actual problems and fixes with getting the EnhancedClient working should be open because it might help other people down the line. The other stuff 🤷

strangely i wasnt able to load an item, I always got an error that NPE because payload is null - during debugging I saw the table content

Not sure what that is about. The data is there but... not there?

@hamburml
Copy link
Contributor Author

hamburml commented Sep 11, 2022

I am back with some news :)

Not sure what that is about. The data is there but... not there?

Yeah, the data was there. I simply forgot the QuarkusInterceptor in the examples... Don't think about it :)

In this repository https://github.com/hamburml/quarkus-amazon-services/tree/feature/DynamoDbEnhancedModule i have a working dynamodb-enhanced module added with the changes of @Nithanim. The tests do still work and I can use it locally. What is important to add both dependencies (dynamodb and dynamodb-enhanced). I have two test-projects here https://github.com/hamburml/dynamodb-enhanced-examples which you can use after you run mvn install in my branch.

What is really strange - if I try to build it against quarkus 2.12.0.Final I always get build errors (cant import AutomaticFeature and ReflectionFeature anymore). If you like I can close this PR and reopen a new one - and link this PR to the other one.

@gsmet
Copy link
Member

gsmet commented Sep 12, 2022

@hamburml you can just push to your branch again.

Can you post the details of the errors you get with 2.12?

@hamburml
Copy link
Contributor Author

@gsmet This is solved thanks to @Nithanim. I will update the branch.

@hamburml hamburml force-pushed the feature/AddDynamoDbEnhancedClient branch from 45c98e6 to 0fe13b4 Compare September 12, 2022 08:23
@hamburml
Copy link
Contributor Author

@gsmet branch is updated

Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

I added a few comments and questions.

I really need an executive summary for the funky things you are doing as I don't really have much time to dig into this myself.

Thanks!

dynamodb-enhanced/deployment/pom.xml Outdated Show resolved Hide resolved
Comment on lines 79 to 84
/**
* Decides if the transformation should be applied. Predominantly this should only happen in
* non-production builds because only a single {@link ClassLoader} is used there making this "fix"
* useless and probably worsens performance. Second, we do not apply the transformation when
* requested through config.
*/
Copy link
Member

Choose a reason for hiding this comment

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

Could you explain what you're trying to do here with this transformation? I had a look at your discussions but couldn't understand what's going on at a quick glance.
Moreover we need a comment that actually explains the rationale of this change. Here you explain why it's not useful in all cases but you have no idea what the purpose is.

Copy link
Contributor Author

@hamburml hamburml Sep 14, 2022

Choose a reason for hiding this comment

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

I try to explain it how I understood it. I do not have a very deep understanding of how Java, Quarkus or GraalVM works under the hood, sorry if it is kinda wonky.

Inside the aws sdk the BeanTableSchema is created and with it some access methods. If you now annotate your bean with @DynamoDbBean the SDK tries to create a Supplier for this bean. To do this the class needs to be loaded by the classloader. Unfortunately this does not work because when it tries to resolve your annotated Bean it uses the classloader of the aws sdk (which does not know your classes) which means you get a NoClassDefFoundError. This does happen during tests and quarkus:dev mode. It is better described in this Issue aws/aws-sdk-java-v2#2604 (the main issue is how the aws sdk v2 does this). Of course when using GraalVM it tries to solve all the reflection magic beforehand so that we can have an executable. Here the same issue exists - during GraalVM build it can not resolve the classes.

So @Nithanim found a workaround to register the classes in the correct classloader via transformers.produce(...). As said we only need this workaround for test, dev-mode and native build. Therefore we check the LaunchMode - if the LaunchMode is not LaunchMode.NORMAL (which is prod), the fix is added (when the jvmTransformation bool is set to true). For native mode the fix is the registerClassesForReflectiveAccess(...) - here we search for all Classes which are Annotated by DynamoDbBean and reflectiveClass.produce(...) them each.

To be honest, I think the jvmTransformation config could be removed - as long as the aws sdk does not fix this issue and we want to use the enhanced client in jvm and native mode, we need to register the classes manually.

Copy link
Member

Choose a reason for hiding this comment

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

OK, I think I have a better understanding of it. We need proper comments in the classes, I'll probably add something once we're done with the heavy lifting.

Comment on lines 10 to 12
/** Apply patch for crashing tests. Reduces performance in JVM mode. */
@ConfigItem(defaultValue = "true")
public boolean jvmTransformation;
Copy link
Member

Choose a reason for hiding this comment

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

Could you make me a summary of what this is supposed to fix?

Copy link
Contributor Author

@hamburml hamburml Sep 14, 2022

Choose a reason for hiding this comment

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

This is only a flag which you can set if you do not want to use the fix when in LaunchMode.DEVELOPMENT and LaunchMode.TEST (in jvm mode, not native).

Copy link
Member

Choose a reason for hiding this comment

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

I agree we should just remove this flag.

- "dynamo"
- "aws"
- "amazon"
guide: "https://quarkus.io/guides/amazon-dynamodb"
Copy link
Member

Choose a reason for hiding this comment

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

This should go away. If it's present in the other extensions we need to remove it. See updated URL a few lines below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we should write a new guide for that and change the URL? Or should the amazon-dynamodb guide adjusted?

@hamburml hamburml force-pushed the feature/AddDynamoDbEnhancedClient branch from 5e754aa to bb2b3a9 Compare September 14, 2022 13:51
Comment on lines 16 to 49
private final DynamoDbEnhancedClient syncEnhancedClient;

private final DynamoDbEnhancedAsyncClient asyncEnhancedClient;

DynamodbEnhancedClientProducer() {
this.syncEnhancedClient = DynamoDbEnhancedClient.builder()
.dynamoDbClient(Arc.container().instance(DynamoDbClient.class).get()).build();
this.asyncEnhancedClient = DynamoDbEnhancedAsyncClient.builder()
.dynamoDbClient(Arc.container().instance(DynamoDbAsyncClient.class).get()).build();
}

@Produces
@ApplicationScoped
public DynamoDbEnhancedClient client() {
if (syncEnhancedClient == null) {
throw new IllegalStateException("The DynamoDbEnhancedClient is required but has not been detected/configured.");
}
return syncEnhancedClient;
}

@Produces
@ApplicationScoped
public DynamoDbEnhancedAsyncClient asyncClient() {
if (asyncEnhancedClient == null) {
throw new IllegalStateException(
"The DynamoDbEnhancedAsyncClient is required but has not been detected/configured.");
}
return asyncEnhancedClient;
}

@PreDestroy
public void destroy() {

}
Copy link
Member

Choose a reason for hiding this comment

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

I think get on Arc.container().instance() will throw if not registered. BTW, you may get rid of Arc with Instance or by injecting the client in the producer. I would favor the latest as the extension do not need to keep a direct reference on the enhanced client instances.

Suggested change
private final DynamoDbEnhancedClient syncEnhancedClient;
private final DynamoDbEnhancedAsyncClient asyncEnhancedClient;
DynamodbEnhancedClientProducer() {
this.syncEnhancedClient = DynamoDbEnhancedClient.builder()
.dynamoDbClient(Arc.container().instance(DynamoDbClient.class).get()).build();
this.asyncEnhancedClient = DynamoDbEnhancedAsyncClient.builder()
.dynamoDbClient(Arc.container().instance(DynamoDbAsyncClient.class).get()).build();
}
@Produces
@ApplicationScoped
public DynamoDbEnhancedClient client() {
if (syncEnhancedClient == null) {
throw new IllegalStateException("The DynamoDbEnhancedClient is required but has not been detected/configured.");
}
return syncEnhancedClient;
}
@Produces
@ApplicationScoped
public DynamoDbEnhancedAsyncClient asyncClient() {
if (asyncEnhancedClient == null) {
throw new IllegalStateException(
"The DynamoDbEnhancedAsyncClient is required but has not been detected/configured.");
}
return asyncEnhancedClient;
}
@PreDestroy
public void destroy() {
}
private final DynamoDbEnhancedClient syncEnhancedClient;
DynamodbEnhancedClientProducer(Instance<DynamoDbClient> syncClientInstance) {
this.syncEnhancedClient = syncClientInstance.isResolvable() ? DynamoDbEnhancedClient.builder()
.dynamoDbClient(syncClientInstance.get()).build() : null;
}
@Produces
@ApplicationScoped
public DynamoDbEnhancedClient client() {
if (syncEnhancedClient == null) {
throw new IllegalStateException("The DynamoDbEnhancedClient is required but has not been detected/configured.");
}
return syncEnhancedClient;
}
@Produces
@ApplicationScoped
public DynamoDbEnhancedAsyncClient asyncClient(DynamoDbAsyncClient client) {
return DynamoDbEnhancedAsyncClient.builder()
.dynamoDbClient(client).build();
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, i can switch to Instance<...> but I can not Inject DynamoDbClient and DynamoDbAsyncClient. If I do that I always get Unable to load region from any of the providers in the chain ....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any idea or is Instance<...> okay?

@hamburml
Copy link
Contributor Author

@gsmet @Nithanim @scrocquesel
I am done. My jvm tests work, jvm debugging work, jvm prod profile works an my native image runs.

What I did:

  • removed the BuildTimeConfig
  • removed the native substitution fix, which is not needed anymore because the "jvm test/debugging fix" does also fix the native image problem (see Add DynamoDB Enhanced Client for DynamoDB Annotations #255 (comment))
  • added @BuildStep(onlyIf = NativeBuild.class) to registerClassesForReflectiveAccess because this is only needed for native

Now it's cleaner and more streamlined. I would really appreciate a review and I hope this can be added. @gsmet You mentioned that you want more comments in the classes - I am open for advices. Or if you like you can add them yourself.

My jvm tests work, jvm debugging work, jvm prod profile works an my native image runs.

Thanks for your help <3

@gsmet
Copy link
Member

gsmet commented Sep 22, 2022

I will have a closer look today and hopefully get it merged. Don't bother about the reported conflicts, I want to rebase, maybe add a few comments and things like that so I'll deal with them when reviewing.

@gsmet
Copy link
Member

gsmet commented Sep 23, 2022

I started fixing a few things. Please don't push.

@gsmet gsmet force-pushed the feature/AddDynamoDbEnhancedClient branch from 4a4465c to 9a8ac37 Compare September 23, 2022 14:12
@gsmet
Copy link
Member

gsmet commented Sep 23, 2022

I'm going to merge this once CI is green as it's long overdue. I only made a couple of small adjustments (mostly adding comments and fixed very minor things).

Once merged, could you either add a paragraph in https://github.com/quarkiverse/quarkus-amazon-services/blob/main/docs/modules/ROOT/pages/amazon-dynamodb.adoc or add a new amazon-dynamodb-enhanced.adoc guide?
I think a simple paragraph in amazon-dynamodb.adoc would be enough as a first step.

@hamburml
Copy link
Contributor Author

Sure, I am happy to contribute :)

@gsmet gsmet merged commit 5a06043 into quarkiverse:main Sep 23, 2022
@gsmet
Copy link
Member

gsmet commented Sep 23, 2022

And merged, thanks everyone.

Once we have a small doc, I'll publish a new release containing the extension.

@Nithanim
Copy link

Great news! Thank you all for your efforts!

@gsmet
Copy link
Member

gsmet commented Sep 26, 2022

@allcontributors add hamburml for code

@allcontributors
Copy link
Contributor

@gsmet

I've put up a pull request to add @hamburml! 🎉

@gsmet
Copy link
Member

gsmet commented Sep 26, 2022

@allcontributors add Nithanim for code

@allcontributors
Copy link
Contributor

@gsmet

I've put up a pull request to add @Nithanim! 🎉

@gsmet
Copy link
Member

gsmet commented Sep 26, 2022

@allcontributors add Nithanim for code

@allcontributors
Copy link
Contributor

@gsmet

I've put up a pull request to add @Nithanim! 🎉

@gsmet
Copy link
Member

gsmet commented Sep 26, 2022

1.3.0 has been released.

@vamsisayimpu
Copy link

#TODO: Quakrus Testing framework uses QuarkusClassLoader, for workaround use this below property in test/app.properties
quarkus.test.flat-class-path=true

@hamburml
Copy link
Contributor Author

hamburml commented Dec 9, 2022

@vamsisayimpu I think this would not solve the native image issues.

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.

None yet

5 participants