[NU-1897] flink-executor and lite-runtime modules: Added compile-time dependency to http-utils #7259
Conversation
arkadius
changed the base branch from
staging
to
componentsApi-remove-netty-dependency
arkadius
changed the title
flink-executor and lite-runtime modules: Added compile-time dependency to http-utils
arkadius
changed the title
flink-executor and lite-runtime modules: Added compile-time dependency to http-utils
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces significant modifications to the Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
docs/MigrationGuide.md (2)
Line range hint
4-4: Add unit tests as mentioned in TODO commentTests should be added to verify the behavior of the formula function, especially given the recent parameter addition.
Would you like me to help create unit tests for this function?
Security update needed: requests 2.26.0 has known vulnerabilities
The pinned version 2.26.0 of requests is affected by two moderate severity vulnerabilities:
- Unintended leak of Proxy-Authorization header (fixed in 2.31.0)
- Session verification bypass vulnerability (fixed in 2.32.0)
Recommend upgrading to the latest stable version 2.32.3 to address these security issues.
🔗 Analysis chain
Line range hint
6-6: Verify security of fixed requests versionThe requests library is pinned to version 2.26.0. Let's verify if this version has any known security vulnerabilities.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check PyPI for latest versions and GitHub for security advisories # Get latest version from PyPI echo "Latest version from PyPI:" curl -s https://pypi.org/pypi/requests/json | jq -r '.info.version' # Check for security advisories echo -e "\nSecurity advisories:" gh api graphql -f query=' { securityVulnerabilities(first: 5, ecosystem: PIP, package: "requests") { nodes { advisory { summary severity publishedAt } vulnerableVersionRange firstPatchedVersion { identifier } } } }'Length of output: 1716
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
build.sbt(5 hunks)docs/Changelog.md(1 hunks)docs/MigrationGuide.md(1 hunks)
🔇 Additional comments (7)
docs/Changelog.md (1)
17-22: LGTM! Changes are well documented.
The changelog entries accurately reflect the dependency cleanup and API enhancement changes:
- Component API enhancement for SpEL template access
- Dependency cleanup:
- Narrowing of async-http-client dependency
- Addition of http-utils compile-time dependency
docs/MigrationGuide.md (1)
Line range hint 1-2: LGTM!
The subtraction function is implemented correctly.
build.sbt (5)
814-823: Addition of httpUtils Dependency
Including httpUtils as a compile-time dependency in the flinkExecutor module ensures consistent Netty versions across the dependency stack (sttp → async-http-client → netty). This prevents NoClassDefFoundError exceptions and reduces the model jar size.
1367-1375: Adding httpUtils to liteEngineRuntime Dependencies
Adding httpUtils as a compile-time dependency in the liteEngineRuntime module is appropriate to maintain consistent Netty versions. This helps avoid potential runtime conflicts and reduces the model jar size.
1562-1575: Updated Dependencies in componentsApi Module
The additions to the dependencies in the componentsApi module enhance functionality and align with project requirements. Ensure that all version variables (e.g., flinkCommonsTextV, catsV, enumeratumV) are correctly defined and up to date.
1710-1713: Including sttp Dependencies in httpUtils Module
Adding the sttp dependencies to the httpUtils module helps maintain consistent library versions and prevents potential runtime errors due to mismatched Netty versions in the dependency stack.
1523-1528: Verify Netty Version Consistency in Test Dependencies
The inclusion of async-http-client-backend-future in the test scope may introduce Netty version conflicts. Please verify that the Netty versions across all dependencies are consistent to prevent runtime issues.
Run the following script to list Netty dependencies and their versions:
✅ Verification successful
Based on the results, I can now generate the final response:
Netty Version Consistency is Already Managed
The codebase already handles Netty version consistency carefully:
- A single
nettyV = "4.1.113.Final"is defined and used across all Netty dependencies - There are explicit comments acknowledging and addressing the sttp -> async-http-client -> netty dependency chain
- The project has proper exclusion rules for conflicting Netty versions (e.g., azure-core-http-netty exclusions)
- Netty reactive streams version is explicitly managed with
nettyReactiveStreamsV = "2.0.12"
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: List Netty dependencies and their versions in the project.
# Search for Netty dependencies in build.sbt.
rg 'netty' build.sbt
Length of output: 1569
Script:
#!/bin/bash
# Let's check the version of netty that async-http-client-backend-future brings in
rg -A 1 'sttpV' build.sbt
rg -A 1 'async-http-client' build.sbt
# Also check for any other potential netty dependencies through transitive dependencies
rg 'val.*=.*".*netty.*"' build.sbt
Length of output: 2360
arkadius
changed the base branch from
componentsApi-remove-netty-dependency
to
staging
arkadius
changed the base branch from
staging
to
componentsApi-remove-netty-dependency
arkadius
force-pushed
the
flinkexecutor-httputils-dependency
branch
from
67cbe70
to
b2d1c20
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
docs/MigrationGuide.md (1)
Line range hint
4-4: Add unit tests as indicated by TODO commentThe TODO comment indicates missing tests. Unit tests are important to verify the behavior of this function, especially given it was recently modified to add a new parameter.
Would you like me to help create unit tests for this function?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
build.sbt(5 hunks)docs/Changelog.md(1 hunks)docs/MigrationGuide.md(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/Changelog.md
🔇 Additional comments (4)
build.sbt (2)
814-823: LGTM! Well-documented dependency management improvement.
The addition of httpUtils as a compile-time dependency to flinkExecutor is a good solution for:
- Resolving NoClassDefFoundError issues caused by conflicting netty versions
- Reducing the size of the model jar bundle
- Ensuring consistent library versions across the dependency stack (sttp -> async-http-client -> netty)
This change establishes a pattern where common HTTP-related dependencies are managed through a shared module, promoting consistency and reducing the risk of version conflicts.
Let's verify the dependency tree to confirm the resolution of netty version conflicts:
✅ Verification successful
LGTM! The dependency management approach is well-implemented and consistent
The verification confirms that:
- The project uses a fixed netty version (4.1.113.Final) across all dependencies
- The
httpUtilsmodule is properly configured as a shared dependency to ensure consistent versions in the stack: sttp -> async-http-client -> netty - Azure dependencies that could introduce conflicting netty versions are properly excluded
- The dependency is correctly added as compile-time to both
flinkExecutorandliteEngineRuntime
The implementation effectively:
- Prevents NoClassDefFoundError by maintaining consistent netty versions
- Reduces jar bundle size through shared dependencies
- Follows good practices in dependency management with proper exclusions and version control
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential netty version conflicts in the dependency tree
# Expected: Only one version of netty should be present
# Check netty versions in the classpath
sbt 'flinkExecutor/dependencyTree' | grep -i netty
# Verify the size reduction in the model jar
ls -lh flinkExecutor/target/scala-*/flinkExecutor.jar
Length of output: 296
Script:
#!/bin/bash
# Let's try a different approach to verify the dependency structure
# First, check if there are any direct netty dependencies declared in build.sbt
rg -A 3 -B 3 "netty" build.sbt
# Then check for any other HTTP client related dependencies
rg -A 3 -B 3 "async-http-client|sttp|http" build.sbt
# Also check for any dependency exclusions related to netty
rg "exclude\(" build.sbt
Length of output: 26909
1367-1375: LGTM! Consistent dependency management approach.
The addition of httpUtils as a compile-time dependency to liteEngineRuntime follows the same pattern as flinkExecutor, ensuring consistent dependency management across different runtime environments.
Let's verify the dependency resolution is consistent with the Flink executor:
✅ Verification successful
Dependency management approach is consistent across the codebase
Based on the analysis of build.sbt, the review comment is correct. The evidence shows:
httpUtilsis consistently used across multiple components, with some using it as a Provided dependency (e.g., in Flink components) and others as a direct dependency where needed- The liteEngineRuntime's dependency on
httpUtilsaligns with the codebase's pattern, particularly considering the comment about managing netty version conflicts - Both Flink and Lite runtimes show careful management of their HTTP-related dependencies, with Flink using provided scope for potential conflict resolution and Lite runtime explicitly managing its HTTP stack (sttp, akka-http)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Compare netty versions between Flink and Lite runtimes
# Expected: Both should use the same version of netty
# Check netty versions in both classpaths
echo "=== Flink Runtime ==="
sbt 'flinkExecutor/dependencyTree' | grep -i netty
echo "=== Lite Runtime ==="
sbt 'liteEngineRuntime/dependencyTree' | grep -i netty
# Compare jar sizes to verify optimization
echo "=== Jar sizes ==="
ls -lh */target/scala-*/flinkExecutor.jar */target/scala-*/liteEngineRuntime.jar 2>/dev/null
Length of output: 529
Script:
#!/bin/bash
# Let's try a different approach by examining the build.sbt file directly
# to verify the dependency management consistency between Flink and Lite runtimes
# First, let's look at the flinkExecutor dependencies
echo "=== Flink Executor Dependencies ==="
rg "lazy val flinkExecutor" -A 20 build.sbt
# Then, let's look at the liteEngineRuntime dependencies
echo "=== Lite Engine Runtime Dependencies ==="
rg "lazy val liteEngineRuntime" -A 20 build.sbt
# Also check for any other httpUtils dependencies to verify consistency
echo "=== Other httpUtils Dependencies ==="
rg "httpUtils" build.sbt
Length of output: 4931
docs/MigrationGuide.md (2)
Line range hint 1-2: LGTM!
The subtraction function is implemented correctly.
Line range hint 6-6: Verify security of pinned requests version
The requests library is pinned to version 2.26.0. Let's verify if this version has any known security vulnerabilities.
Describe your changes
Checklist before merge
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores