Alternate entry point without Lambda #308
Conversation
johnttompkins
left a comment
There was a problem hiding this comment.
Didn't go into the Wrapper.java class yet just an initial go through - are there any large changes from the lambdawrapper class or is it just moving the contents into a new class for inheritance purposes?
| } | ||
| {% else %} | ||
| public static void main(String[] args) throws IOException { | ||
| if (args.length != 1){ |
There was a problem hiding this comment.
one arg is just the stringified handler request?
Updated the description with a link to the diff |
wbingli
left a comment
There was a problem hiding this comment.
Generally looks good.
Other question is about backward compatibility on the PyPi and java maven version bump. A common scenario is:
- What if customer upgrade the
cloudformation-cliPyPi only? Will it break existing build? - What if customer upgrade the
cloudformation-cli-java-pluginPyPI only without upgrading maven version? will it break the build? - What if customer only upgrade
aws-cloudformation-rpdk-java-pluginmaven plugin? will it break anything? They may use<version>[2.0.0, 3.0.0)</version>in the maven for always pull the latest version? Will it need a major version bump?
| final MetricsPublisher providerMetricsPublisher, | ||
| final SchemaValidator validator, | ||
| final Serializer serializer, | ||
| final SdkHttpClient httpClient) { |
There was a problem hiding this comment.
Why we need to add this SdkHttpClient as one of Wrapper constructor? We are using a shared the HTTP client for all client to share connection pool etc. It should be just to use a default static HTTP client.
There was a problem hiding this comment.
I did not touch this code, I just copied it from the existing lambdaWrapper. From what I could tell, this was done only so that we could mock the httpClient during testing easily
|
|
||
| } finally { | ||
| if (handlerResponse != null) { | ||
| publishExceptionCodeAndCountMetric(request == null ? null : request.getAction(), handlerResponse.getErrorCode(), |
There was a problem hiding this comment.
There is more new changes on the LabmdaWrapper. Make sure merge the latest change after the refactoring, otherwise it will cause regression. A recent PR will have new changes on this metric publish.
There was a problem hiding this comment.
Since I'm refactoring, any changes to the LambdaWrapper will cause merge conflicts so I will do this at the end and make sure the merge is also reviewed. I just don't want to keep fixing the merge conflicts while also making code changes so it's not as confusing looking at the diffs
python/rpdk/java/codegen.py
Outdated
| project.overwrite(path, contents) | ||
|
|
||
| # write generated handler integration with ExecutableWrapper | ||
| path = src / "ExecutableHandlerWrapper.java" |
There was a problem hiding this comment.
Generate this new class will cause build break if customer doesn't update the java plugin version. And it happens quite common as update java plugin needs to update the pom.xml.
One way we use during 2.0 upgrade is to check the java plugin version, and give an error if doesn't match. I think you may can leverage the _get_java_plugin_dependency_version to check only generate this class if version is 2.1.0 or above. Assuming we will bump the java plugin version to 2.1.0.
There was a problem hiding this comment.
Good call out. Current version is 2.0.2 so I just said anything greater than 2.0.3 so we can bump to that or 2.1
…ed and move start/end response to handlerwrapper.java class instead
fc8ff84 to
c615d2d
Compare
| ) | ||
| if ( | ||
| java_plugin_dependency_version | ||
| >= MINIMUM_JAVA_DEPENDENCY_VERSION_EXECUTABLE_HANDLER_WRAPPER |
… for backwards compatible unit tests excludes
Description of changes: This PR introduces another entry point to the plugin that completely removes the lambda dependency. This allows handler code to run on any platform and will not be limited to just AWS Lambda environments.
To do this, a new parent wrapper class was introduced that holds all the core logic to handle CRUDL requests. This new parent class abstracts away the lambda logger into a generic log publisher so that both the lambda wrapper and the new executable wrapper could use their own loggers. Changes to the cloudwatch metrics and log helper classes were also made in order to use the generic log publisher.
Diff for the wrapper: https://www.textcompare.org/index.html?id=5f68caea124d230017b1d14f
There is a new child class (ExecutableWrapper.java) that serves as the base for the entry point without the lambda dependency executable wrapper logs to both a file and the console. Both this new child class and the lambda wrapper class call the base wrapper's processRequest method after setting up the platform logger.
The last changes are to the codegen. It adds logging configuration and will create a separate java class that serves as the alternate entry point.
Tested these changes with a personal resource.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.