Bazel migration for Flink module #237
Conversation
WalkthroughThis pull request enhances the Flink project's build configuration by introducing three new Scala library and test suite definitions in Changes
Possibly related PRs
Suggested reviewers
Poem
Warning Review ran into problems🔥 ProblemsGitHub Actions: Resource not accessible by integration - https://docs.github.com/rest/actions/workflow-runs#list-workflow-runs-for-a-repository. Please grant the required permissions to the CodeRabbit GitHub App under the organization or repository settings. 🪧 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
🧹 Nitpick comments (1)
flink/BUILD.bazel (1)
26-101: Consolidate test dependenciestest-lib and test targets have duplicate dependencies.
# Create a variable at the top of the file +TEST_COMMON_DEPS = [ + ":lib", + "//online:lib", + "//api:thrift", + # ... rest of common dependencies +] scala_library( name = "test-lib", srcs = glob(["src/test/**/*.scala"]), visibility = ["//visibility:public"], - deps = [ - ":lib", - "//online:lib", - # ... current deps - ], + deps = TEST_COMMON_DEPS, ) scala_test_suite( name = "test", srcs = glob(["src/test/**/*.scala"]), visibility = ["//visibility:public"], - deps = [ - ":lib", - "//online:lib", - # ... current deps - ], + deps = TEST_COMMON_DEPS, )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (2)
flink/BUILD.bazel(1 hunks)tools/build_rules/dependencies/maven_repository.bzl(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: no_spark_scala_tests
🔇 Additional comments (3)
tools/build_rules/dependencies/maven_repository.bzl (1)
54-61: Consider upgrading to Flink 1.18.1Flink 1.17.0 is not the latest stable version.
✅ Verification successful
Current Flink version 1.17.0 is acceptable
While Flink 1.20.0 is available, 1.17.0 is secure and consistently used across the codebase. Upgrade at your convenience.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check latest Flink version and CVEs curl -s https://api.github.com/repos/apache/flink/releases/latest | jq -r '.tag_name' gh api graphql -f query=' { securityVulnerabilities(first: 5, ecosystem: MAVEN, package: "org.apache.flink:flink-core") { nodes { advisory { summary severity } vulnerableVersionRange } } }'Length of output: 614
Script:
#!/bin/bash # Check latest Flink version from Maven Central curl -s "https://search.maven.org/solrsearch/select?q=g:org.apache.flink+AND+a:flink-core&core=gav&rows=1&wt=json" | jq -r '.response.docs[0].v' # Verify if this version is used elsewhere in the codebase rg "org\.apache\.flink.*1\." -g '*.{xml,bzl,bazel}'Length of output: 950
Script:
#!/bin/bash # Check latest stable 1.x version from Maven Central curl -s "https://search.maven.org/solrsearch/select?q=g:org.apache.flink+AND+a:flink-core+AND+v:1.*&core=gav&rows=5&wt=json" | jq -r '.response.docs[].v' | grep -v 'preview\|alpha\|beta\|rc' | sort -V | tail -n1Length of output: 220
flink/BUILD.bazel (2)
1-24: LGTM: Main library configurationDependencies and source globs are well-structured.
66-66: Verify test coveragePR description indicates no new tests were added.
Also applies to: 67-67
✅ Verification successful
Test coverage verified
The flink module has comprehensive test coverage with multiple test files in the correct location. The glob pattern in BUILD.bazel is properly configured to include these tests.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check test files echo "Test files found:" fd -e scala . src/test/Length of output: 167
Script:
#!/bin/bash # Find all Scala test files in the repository echo "=== Searching for Scala files with 'test' in path ===" fd -e scala -I test echo -e "\n=== Searching for test annotations/classes in Scala files ===" rg -l "(@Test|extends.*Test|class.*Test)" -g "*.scala"Length of output: 12971
flink/BUILD.bazel
Outdated
| name = "test", | ||
| srcs = glob(["src/test/**/*.scala"]), | ||
| visibility = ["//visibility:public"], | ||
| deps = [ |
There was a problem hiding this comment.
there seems to be a lot of repetition across targets - i would split these into two python arrays - flink_deps, flink_test_deps and re-use in three places.
flink/BUILD.bazel
Outdated
| "//aggregator:lib", | ||
| "//tools/build_rules/spark:spark-exec", | ||
| # Libraries | ||
| scala_artifact("org.apache.flink:flink-streaming-scala"), |
There was a problem hiding this comment.
when we build the flink jar - which of these modules are included and which ones are excluded / treated as provided? We'll want to mimic the sbt assembly behavior here (not blocking this pr but we'll need to address / tackle)
There was a problem hiding this comment.
That's a good point. I think by default bazel includes all dependency jars in the fat jar and we would need additional config setup to replicate excluded/provided behavior in sbt which i'm working as part of separate PR.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
flink/BUILD.bazel (3)
26-26: Restrict glob pattern to prevent accidental inclusion.Add exclusions for temporary/hidden files:
glob(["src/main/**/*.scala"], exclude = [".*", "*~"])
52-58: Optimize ScalaTest dependencies.
scalatestalready includes core modules. Remove redundant dependencies:- scala_artifact("org.scalatest:scalatest-matchers-core"), - scala_artifact("org.scalatest:scalatest-core"), scala_artifact("org.scalatest:scalatest"),
67-79: Simplify test target structure.test-lib and test suite compile same sources. Consider:
- Remove test-lib and move deps to test suite, or
- Make test suite use test-lib's classes instead of recompiling sources
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
flink/BUILD.bazel(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: no_spark_scala_tests
🔇 Additional comments (2)
flink/BUILD.bazel (2)
1-22: Reduce dependency duplication between flink_deps and flink_test_deps.Create a base deps list and extend it for tests:
base_flink_deps = [ scala_artifact("org.apache.flink:flink-streaming-scala"), # ... common deps ] flink_deps = base_flink_deps flink_test_deps = base_flink_deps + [ # test-only deps ]
28-38: Clarify runtime vs provided dependencies.Need to specify which dependencies should be bundled vs provided in the final jar.
## Summary Migrated `Flink` module to Bazel from sbt. ## Checklist - [ ] Added Unit Tests - [x] Covered by existing CI - [ ] Integration tested - [ ] Documentation update <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Added Scala library definitions for the main application and testing. - Introduced new Apache Flink dependencies for streaming, metrics, and client interactions. - **Chores** - Updated build configuration to support a modular Scala project structure. - Enhanced Maven repository with additional Flink-related libraries. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
## Summary Migrated `Flink` module to Bazel from sbt. ## Checklist - [ ] Added Unit Tests - [x] Covered by existing CI - [ ] Integration tested - [ ] Documentation update <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Added Scala library definitions for the main application and testing. - Introduced new Apache Flink dependencies for streaming, metrics, and client interactions. - **Chores** - Updated build configuration to support a modular Scala project structure. - Enhanced Maven repository with additional Flink-related libraries. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
## Summary Migrated `Flink` module to Bazel from sbt. ## Checklist - [ ] Added Unit Tests - [x] Covered by existing CI - [ ] Integration tested - [ ] Documentation update <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Added Scala library definitions for the main application and testing. - Introduced new Apache Flink dependencies for streaming, metrics, and client interactions. - **Chores** - Updated build configuration to support a modular Scala project structure. - Enhanced Maven repository with additional Flink-related libraries. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
## Summary Migrated `Flink` module to Bazel from sbt. ## Checklist - [ ] Added Unit Tests - [x] Covered by existing CI - [ ] Integration tested - [ ] Documentation update <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Added Scala library definitions for the main application and testing. - Introduced new Apache Flink dependencies for streaming, metrics, and client interactions. - **Chores** - Updated build configuration to support a modular Scala project structure. - Enhanced Maven repository with additional Flink-related libraries. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
## Summary Migrated `Flink` module to Bazel from sbt. ## Cheour clientslist - [ ] Added Unit Tests - [x] Covered by existing CI - [ ] Integration tested - [ ] Documentation update <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Added Scala library definitions for the main application and testing. - Introduced new Apache Flink dependencies for streaming, metrics, and client interactions. - **Chores** - Updated build configuration to support a modular Scala project structure. - Enhanced Maven repository with additional Flink-related libraries. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Summary
Migrated
Flinkmodule to Bazel from sbt.Checklist
Summary by CodeRabbit
New Features
Chores