Skip to content

Conversation

yualexan
Copy link
Contributor

@yualexan yualexan commented Mar 2, 2023

Description of changes:

Use java11 in all samples. As part of this PR, also updates dependencies and adds a bit of functionality to each handler to show basic usage patterns. Wrote unit tests to accompany each handler.

I manually tested each handler through the deployment scripts provided.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

- Upgrade runtime from java8 to java11
- Increase code memory size from 512MB -> 2048MB
- Tested all steps and function still runs properly
- Added a bit more functionality to each handler
- Added proper unit testing for each handler; all tests pass
- Maven hack to exclude META-INF from shaded jar, avoiding a warning
- Tested handlers to ensure they work as advertised
- Add better functionality to each handler event.
- Write one unit test per handler, using aws-lambda-java-tests library.
- Tested each handler to ensure each still works.
- Cleanup of blank-java and java-basic code.
- Instead of SLF4J logger, use Context's logger (removes SLF4J dependency)
- However, during unit testing, still requires SLF4J. Tests still show this logging option.
- Verified that Maven/Gradle builds don't generate SLF4J warnings.
- Use Context logger for all handlers, removing SLF4J dependency
- Unit testing still requires SLF4J dependency to show this logging option
- Tested all handlers manually, minor updates to README
- Use Context logger in all handlers, removing SLF4J dependency
- Unit testing still requires SLF4J logging
- Included missing CloudFront option in 3-invoke.sh script
- Manually deployed and tested all handlers; all work as normal
<artifactId>log4j-slf4j18-impl</artifactId>
<version>[2.17.1,)</version>
<groupId>org.slf4j</groupId>
<artifactId>slf4j-nop</artifactId>
Copy link
Contributor

Choose a reason for hiding this comment

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

This dependency silently discards log messages. I don't think it's a good idea to use it for an example.

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 actually want to remove SLF4J from the dependencies and just use the context logger (SLF4J is required for the testing to work, so I include it for test context only). However, I can't do so because I think another AWS dependency that is pulling in SLF4J somewhere, causing this warning in logs (https://stackoverflow.com/questions/7421612/slf4j-failed-to-load-class-org-slf4j-impl-staticloggerbinder). The warning is resolved with this dependency. That being said, it's just a warning but I wanted all the output and logs to look as clean as possible.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be better to include a real implementation of slf4j backend. sl4j-simple for example.

- AWSXrayWriteOnlyAccess
- AWSLambdaVPCAccessExecutionRole
Tracing: Active
Layers:
Copy link
Contributor

Choose a reason for hiding this comment

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

I wouldn't make examples of Lambda functions using Layers for application dependencies

Copy link
Member

Choose a reason for hiding this comment

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

Why not?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It'd be good to provide one example of this, and I think blank-java is a good candidate for it. Some of the other languages in this repo show this pattern as well (Node, Python).

Copy link
Contributor

Choose a reason for hiding this comment

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

If you provide an example of this, it absolutely should be in a separate example. This is not what I would expect from 'blank'. I would call it 'app-dependency-layer' or something similar.

Copy link
Member

Choose a reason for hiding this comment

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

Blank does not mean "empty". You can get an empty function from the management console. It's a sample for demonstrating and testing service features. Layers is an important feature, no?

Copy link
Member

@mwunderl mwunderl left a comment

Choose a reason for hiding this comment

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

Tested 3 apps with included build and deploy scripts.

$ gradle --version
------------------------------------------------------------
Gradle 8.0.2
------------------------------------------------------------

Build time:   2023-03-03 16:41:37 UTC
Revision:     7d6581558e226a580d91d399f7dfb9e3095c2b1d

Kotlin:       1.8.10
Groovy:       3.0.13
Ant:          Apache Ant(TM) version 1.10.11 compiled on July 10 2021
JVM:          11.0.18 (Ubuntu 11.0.18+10-post-Ubuntu-0ubuntu120.04.1)
OS:           Linux 5.15.90.1-microsoft-standard-WSL2 amd64

@yualexan
Copy link
Contributor Author

I'll merge this to update samples to Java 11. For the discussion on layers, we'll re-evaluate creating a separate example that illustrates this separately. We should also bring the Lambda layers team into that discussion.

@yualexan yualexan merged commit ec207b8 into main Mar 17, 2023
@yualexan yualexan deleted the use-java-11 branch March 17, 2023 21:13
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.

3 participants