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

Ship StepFunction JSONata dependency by default #12110

Merged
merged 8 commits into from
Jan 9, 2025

Conversation

joe4dev
Copy link
Member

@joe4dev joe4dev commented Jan 7, 2025

Motivation

A support case raised the issue that StepFunctions failed with an SSL error when downloading the JVM 11 dependency used by the JSONata feature. This blocks usage of the JSONata feature for customers behind a proxy in the default configuration.

Alternatively, we need to provide comprehensive documentation on configuring proxies with all possible LocalStack URLs downloaded dynamically.

Changes

  • Create an installable package plugin jpype-jsonata to make the JSONata package installable via lpm
  • Remove unnecessary Jackson dependency (side effect from copying the event-ruler installer). Tested locally with the LocalStack dev run script and the SFN test test_base_map_from_input It turns out that the Jackson dependencies are needed, but it was implicit and unclear why. Added a comment to clarify and re-added the dependency
  • 👉 Change JRE version to unify with dynamodb-local
  • Move the package installer to the top-level directory for easy discovery following our convention
  • Fix some typos and document JPype limitation
  • sneaky: clarify dev run help as a follow-up to LDR: selectable local mounts #12092 (cc @simonrw )

Implications

Shipping jpype-jsonata increases the size of the LocalStack image by ~2.3MB (477MB+2.3MB => +0.4%) because of the following dependencies:

  • jsonata jar: 166kb
  • jackson-databind jar: 1.6MB
    • jackson-annotations jar: 74kb
    • jackson-core jar: 583kb
  • JRE 11: 60MB. JPype depends on Java and the default JRE 11 is not re-using the same JRE 21 already shipped with dynamodb local (which depends on JRE 17+). We can save this 60MB if we share JRE with another dependency.

@viren-nadkarni @alexrashed @dominikschubert What do you think? Should we unify the JRE versions or live with the +60MB?

  • JPype should work with JRE 21 based on my testing with JRE 23
  • We could add self.java_version = "21" (🚧 implicit alignment) or change the default version (bigger blast radius)~~

@joe4dev joe4dev added the semver: patch Non-breaking changes which can be included in patch releases label Jan 7, 2025
@joe4dev joe4dev self-assigned this Jan 7, 2025
@@ -66,7 +66,7 @@
"--mount-source/--no-mount-source",
is_flag=True,
default=True,
help="Mount source files from localstack, localstack-ext, and moto into the container.",
help="Mount source files from localstack and localstack-ext. Use --local-packages for optional dependencies such as moto.",
Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you!

Copy link

github-actions bot commented Jan 7, 2025

LocalStack Community integration with Pro

    2 files  ±0      2 suites  ±0   1h 55m 51s ⏱️ + 2m 47s
3 943 tests +8  3 634 ✅ +6  309 💤 +2  0 ❌ ±0 
3 945 runs  +8  3 634 ✅ +6  311 💤 +2  0 ❌ ±0 

Results for commit c3b502c. ± Comparison against base commit b7ed5bc.

♻️ This comment has been updated with latest results.

@viren-nadkarni
Copy link
Member

12% increase is image size is substantial.

JPype depends on Java and the default JRE 11 is not re-using the same JRE 21 already shipped with dynamodb local (which depends on JRE 17+)

I'm confused by this line. If JPype works with JRE 21, why is JRE 11 required?

@joe4dev
Copy link
Member Author

joe4dev commented Jan 8, 2025

@viren-nadkarni

12% increase is image size is substantial.

I fully agree and therefore trying out everything to avoid it 🤞

If JPype works with JRE 21, why is JRE 11 required?

I'm still experimenting 🚧 My worries are based on our experience with JPype, which only supports one running JVM instance. I'm trying to validate if that only applies to multiple JPype instances (which we knew before and blew up around the v4 release) or also in interaction with other JREs used by packages such as dynamodb-local.

@joe4dev
Copy link
Member Author

joe4dev commented Jan 9, 2025

It seems that sharing the JRE 21 with dynamodb-local works 🟢 and we don't need to increase the image size significantly 🙌
Just waiting for the ext tests to validate ...

@alexrashed
Copy link
Member

alexrashed commented Jan 9, 2025

@viren-nadkarni @alexrashed @dominikschubert What do you think? Should we unify the JRE versions or live with the +60MB?

@joe4dev I fully agree with @viren-nadkarni that the additional image size is very substantial and should definitely be avoided if possible. I would propose in the short term to upgrade the Java version just for this package. A global upgrade should be handled in a different PR solely focusing on that one upgrade. But I can see that you are already on this :) 🚀
If it would speed up the availability of the LPM package, I would even propose to extract the Dockerfile modifications in question from the first iteration of the PR.

@viren-nadkarni What would you propose in the long run? How should the default version be handled? I remember this was a discussion when the Java installer has been introduced (#11139).

@joe4dev joe4dev marked this pull request as ready for review January 9, 2025 12:58
@joe4dev joe4dev requested a review from viren-nadkarni January 9, 2025 12:59
Copy link
Member

@alexrashed alexrashed left a comment

Choose a reason for hiding this comment

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

Awesome! Thanks a lot for jumping on this issue / making sure that we include all stepfunctions dependencies by default! The changes are looking great, thanks especially for taking care that the docker image size does not increase too much by unifying the java version used to be the same as for DynamoDB (which is also included by default). 💯
The changes are looking great! :shipit:

@joe4dev joe4dev merged commit eb6e3dc into master Jan 9, 2025
35 checks passed
@joe4dev joe4dev deleted the ship-stepfunction-jsonata-dependency branch January 9, 2025 13:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver: patch Non-breaking changes which can be included in patch releases
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants