Bazel migration for hub module #242
Conversation
WalkthroughThis pull request introduces a comprehensive refactoring of dependency management across multiple Bazel build files. The changes focus on centralizing library dependencies, adding new Scala and Maven artifacts, and standardizing how dependencies are defined and referenced. The modifications span several projects, including hub, flink, service, and service_commons, with a consistent approach to dependency management. 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: 2
🧹 Nitpick comments (8)
tools/build_rules/dependencies/maven_repository.bzl (1)
86-94: Standardize Google Cloud dependency versions.Different versions of Google Cloud libraries may cause compatibility issues.
Consider aligning versions:
- google-cloud-bigquery: 2.42.0
- google-cloud-bigtable: 2.41.0
- google-cloud-pubsub: 1.131.0
- google-cloud-dataproc: 4.52.0
flink/BUILD.bazel (1)
64-101: Reduce dependency duplication.Consider extracting common test dependencies into a separate
.bzlfile to avoid duplication between test-lib and test targets.hub/BUILD.bazel (1)
57-71: Clarify testing strategy.Consider standardizing on either JUnit or ScalaTest to maintain consistency.
cloud_gcp/BUILD.bazel (2)
17-22: Optimize cloud dependencies.Consider splitting cloud services into separate targets to reduce binary size for services that don't need all GCP features.
85-99: Review reflection usage.Large number of --add-opens flags suggests heavy reflection. Consider reducing reflection usage for better maintainability.
service/src/test/java/ai/chronon/service/handlers/FetchHandlerTest.java (3)
78-82: Unnecessary HashMap wrapper around Map.of()The
new HashMap<>()wrapper is redundant sinceMap.of()already creates an immutable map.- Map<String, Object> featureMap = new HashMap<>(Map.of( + Map<String, Object> featureMap = Map.of( "feature_1", 12, "feature_2", 23.3, "feature_3", "USD" - )); + );
197-200: Unnecessary ArrayList wrapper around List.of()The
new ArrayList<>()wrapper is redundant sinceList.of()already creates an immutable list.- List<JavaResponse> mockResponseList = new ArrayList<>(List.of( + List<JavaResponse> mockResponseList = List.of( mockResponse1, mockResponse2 - )); + );
285-285: Remove duplicate semicolonThere's a duplicate semicolon at the end of
async.complete();;.- async.complete();; + async.complete();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (9)
WORKSPACE(1 hunks)cloud_gcp/BUILD.bazel(1 hunks)flink/BUILD.bazel(1 hunks)hub/BUILD.bazel(1 hunks)service/BUILD.bazel(1 hunks)service/src/test/java/ai/chronon/service/handlers/FetchHandlerTest.java(5 hunks)spark/BUILD.bazel(2 hunks)tools/build_rules/dependencies/maven_repository.bzl(2 hunks)tools/build_rules/dependencies/spark_repository.bzl(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: table_utils_delta_format_spark_tests
- GitHub Check: mutation_spark_tests
- GitHub Check: join_spark_tests
- GitHub Check: fetcher_spark_tests
- GitHub Check: no_spark_scala_tests
- GitHub Check: other_spark_tests
🔇 Additional comments (7)
WORKSPACE (1)
35-37: Verify compatibility with downgraded rules_jvm_external.Downgrading from 6.6 to 4.5 might impact newer dependencies.
✅ Verification successful
Downgrade to rules_jvm_external 4.5 appears safe.
The codebase uses standard Java/Scala dependencies without complex version constraints or custom configurations. Version 4.5 is stable and supports all observed usage patterns.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any dependency resolution errors in Bazel build find . -name "*.bazel" -o -name "BUILD" | while read -r file; do echo "Checking dependencies in $file" grep -A 5 "maven_artifact" "$file" doneLength of output: 24033
Script:
#!/bin/bash # Check for maven dependency version constraints fd "maven_install.json|maven.lock|dependencies.yaml|pom.xml" --type f --exec cat {}Length of output: 87
tools/build_rules/dependencies/maven_repository.bzl (1)
63-69: LGTM! Flink dependencies are consistently versioned.All Flink dependencies use version 1.17.0.
flink/BUILD.bazel (1)
11-12: Verify Flink dependency versions.Ensure all Flink dependencies use consistent versions to prevent runtime conflicts.
✅ Verification successful
All Flink dependencies use consistent version 1.17.0
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for mixed Flink versions rg "org.apache.flink:[^:]+:[^:]+" | sort | uniq -cLength of output: 845
Script:
#!/bin/bash # Check scala artifacts rg "scala_artifact.*org.apache.flink:" -A 1Length of output: 786
hub/BUILD.bazel (1)
11-15: Verify Vert.x dependency versions.Check compatibility between Vert.x modules to prevent runtime issues.
✅ Verification successful
All Vert.x dependencies are aligned at version 4.5.10
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for mixed Vert.x versions rg "io.vertx:[^:]+:[^:]+" | sort | uniq -cLength of output: 753
spark/BUILD.bazel (1)
24-25: Verify logging configuration.Adding log4j dependencies without configuration file might cause logging issues.
✅ Verification successful
Logging is properly configured programmatically
The log4j configuration is handled in
SparkSessionBuilderusingConfigurationBuilder, which is a better approach than static configuration files.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for log4j configuration files fd -g "*log4j*.properties" -g "*log4j*.xml"Length of output: 144
Script:
#!/bin/bash # Find log4j config files fd -e properties -e xml -e yml -e yaml -e json | grep -i "log4j" # Check for programmatic configuration rg -i "LogManager|ConfigurationBuilder|XmlConfiguration|PropertiesConfiguration" -A 2Length of output: 900
service/src/test/java/ai/chronon/service/handlers/FetchHandlerTest.java (2)
180-184: Same redundant HashMap wrapper in multiple mapsSimilar to the previous comment, remove redundant HashMap wrappers.
Also applies to: 186-190
222-225: Same redundant ArrayList wrapper in multiple listsSimilar to the previous comment, remove redundant ArrayList wrappers.
Also applies to: 256-259, 280-283
|
|
||
| "org.apache.curator:apache-curator:5.5.0", |
There was a problem hiding this comment.
💡 Codebase verification
Remove unused Curator dependency
The Apache Curator dependency appears unnecessary as:
- No direct Curator API usage in codebase
- Existing ZooKeeper usage is managed by Kafka
🔗 Analysis chain
LGTM! Verify ZooKeeper compatibility.
Apache Curator 5.5.0 is compatible with Spark 3.5.1.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if any Spark configurations use ZooKeeper
rg -l "zookeeper" --type scala --type java --type properties
Length of output: 95
Script:
#!/bin/bash
# Find ZooKeeper/Curator usage
rg -l -i "zookeeper|curator"
# Search for Curator API patterns
ast-grep --pattern 'new CuratorFramework$_'
ast-grep --pattern 'CuratorFrameworkFactory.$_'
Length of output: 197
Script:
#!/bin/bash
# Check docker-compose ZooKeeper config
cat docker-compose.yml
Length of output: 2038
| java_library( | ||
| name = "test-lib", | ||
| srcs = glob(["src/test/**/*.java"]), | ||
| visibility = ["//visibility:public"], | ||
| deps = [ | ||
| ":lib", | ||
| "//online:lib", | ||
| "//service_commons:lib", | ||
| # Libraries | ||
| maven_artifact("io.vertx:vertx-core"), | ||
| maven_artifact("io.vertx:vertx-web"), | ||
| maven_artifact("io.vertx:vertx-web-client"), | ||
| # Testing | ||
| maven_artifact("io.vertx:vertx-junit5"), | ||
| maven_artifact("io.vertx:vertx-unit"), | ||
| maven_artifact("org.mockito:mockito-core"), | ||
| maven_artifact("org.junit.jupiter:junit-jupiter-api"), | ||
| maven_artifact("junit:junit"), | ||
| ], | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consolidate duplicate test dependencies.
Move common dependencies to test-lib and reference only test-lib in test target.
java_test_suite(
name = "test",
srcs = glob(["src/test/**/*.java"]),
visibility = ["//visibility:public"],
runner = "junit5",
deps = [
":lib",
":test-lib",
- "//online:lib",
- # Libraries
- maven_artifact("io.vertx:vertx-core"),
- maven_artifact("io.vertx:vertx-web"),
- maven_artifact("io.vertx:vertx-web-client"),
- # Testing
- maven_artifact("io.vertx:vertx-junit5"),
- maven_artifact("io.vertx:vertx-unit"),
- maven_artifact("org.mockito:mockito-core"),
- maven_artifact("org.junit.jupiter:junit-jupiter-api"),
- maven_artifact("junit:junit"),
maven_artifact("org.junit.platform:junit-platform-launcher"),
maven_artifact("org.junit.platform:junit-platform-reporting"),
],
)Also applies to: 47-69
WORKSPACE
Outdated
| strip_prefix = "rules_jvm_external-6.6", | ||
| sha256 = "3afe5195069bd379373528899c03a3072f568d33bd96fe037bd43b1f590535e7", | ||
| url = "https://github.com/bazel-contrib/rules_jvm_external/releases/download/6.6/rules_jvm_external-6.6.tar.gz" | ||
| strip_prefix = "rules_jvm_external-4.5", |
There was a problem hiding this comment.
Not needed. Removed after rebase.
cloud_gcp/BUILD.bazel
Outdated
| "//spark:lib", | ||
| "//tools/build_rules/spark:spark-exec", | ||
| # Libraries | ||
| scala_artifact("org.scala-lang.modules:scala-java8-compat"), |
There was a problem hiding this comment.
factor out common deps and re-use
nikhil-zlai
left a comment
nikhil-zlai
left a comment
There was a problem hiding this comment.
Factor out common deps and re-use them across targets - atleast within the same build file.
flink/BUILD.bazel
Outdated
| "//aggregator:lib", | ||
| "//tools/build_rules/spark:spark-exec", | ||
| # Libraries | ||
| scala_artifact("org.apache.flink:flink-streaming-scala"), |
piyush-zlai
left a comment
piyush-zlai
left a comment
There was a problem hiding this comment.
Couple of minor nits
cloud_gcp/BUILD.bazel
Outdated
| "//online:lib", | ||
| "//spark:lib", | ||
| "//tools/build_rules/spark:spark-exec", | ||
| # Libraries |
There was a problem hiding this comment.
like Nikhil mentioned earlier, probably worth creating a scala lib with some deps and re-using them..
| put("feature_2", 23.3); | ||
| put("feature_3", "USD"); | ||
| }}; | ||
| Map<String, Object> featureMap = new HashMap<>(Map.of( |
There was a problem hiding this comment.
nit - the HashMap(Map.of) might be redundant. You can just do the Map.of
| put("feature_2", 23.3); | ||
| put("feature_3", "USD"); | ||
| }}; | ||
| Map<String, Object> featureMap1 = new HashMap<>(Map.of( |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
tools/build_rules/dependencies/maven_repository.bzl(1 hunks)
🔇 Additional comments (1)
tools/build_rules/dependencies/maven_repository.bzl (1)
95-98: LGTM!Circe dependencies are properly versioned and include all necessary components.
| "io.vertx:vertx-micrometer-metrics:4.5.10", | ||
| "io.vertx:vertx-junit5:4.5.10", | ||
| "io.vertx:vertx-unit:4.5.10", | ||
| "io.vertx:vertx-unit:4.5.10", |
There was a problem hiding this comment.
Remove duplicate dependency.
io.vertx:vertx-unit:4.5.10 is already declared on line 92.
- "io.vertx:vertx-unit:4.5.10",
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tools/build_rules/prelude_bazel (1)
43-53: Optimize test dependencies to avoid duplicationConsider creating a base dependency list that's shared between main and test configurations.
+_FLINK_BASE_DEPS = [ + scala_artifact("org.apache.flink:flink-streaming-scala"), + scala_artifact("org.apache.flink:flink-scala"), + maven_artifact("org.apache.flink:flink-streaming-java"), + maven_artifact("org.apache.flink:flink-core"), +] _FLINK_DEPS = _FLINK_BASE_DEPS + [ maven_artifact("org.apache.flink:flink-metrics-dropwizard"), maven_artifact("org.apache.flink:flink-metrics-core"), maven_artifact("org.apache.flink:flink-clients"), maven_artifact("org.apache.flink:flink-yarn"), ] -_FLINK_TEST_DEPS = [ - # Libraries - scala_artifact("org.apache.flink:flink-streaming-scala"), - scala_artifact("org.apache.flink:flink-scala"), - maven_artifact("org.apache.flink:flink-streaming-java"), - maven_artifact("org.apache.flink:flink-core"), +_FLINK_TEST_DEPS = _FLINK_BASE_DEPS + [ maven_artifact("org.apache.flink:flink-runtime"), # Testing maven_artifact("org.apache.flink:flink-test-utils"), maven_artifact("org.apache.flink:flink-runtime-tests"), ]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (5)
flink/BUILD.bazel(2 hunks)hub/BUILD.bazel(1 hunks)service/BUILD.bazel(2 hunks)service_commons/BUILD.bazel(2 hunks)tools/build_rules/prelude_bazel(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- service/BUILD.bazel
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: no_spark_scala_tests
🔇 Additional comments (7)
service_commons/BUILD.bazel (2)
Line range hint
5-15: Clean dependency consolidation using _VERTX_DEPS
18-18: Clean test dependency consolidation using _VERTX_TEST_DEPSflink/BUILD.bazel (2)
Line range hint
5-15: Clean dependency consolidation using _FLINK_DEPS
Line range hint
18-45: Clean test dependency consolidation using _FLINK_TEST_DEPStools/build_rules/prelude_bazel (1)
63-71: Similar optimization needed for Vertx test dependencieshub/BUILD.bazel (2)
1-24: Clean dependency organization using centralized lists
61-66: Add unit tests as mentioned in PR objectivesPR indicates unit tests haven't been added yet. Please implement tests for the hub module functionality.
## Summary Migrated `hub` 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 ## Release Notes - **Dependency Management** - Added new dependency lists for Flink, Vert.x, and Circe libraries - Consolidated and standardized dependency management across multiple projects - Introduced new Maven artifact dependencies for Circe and Vert.x - **Build Configuration** - Refactored dependency references in multiple `BUILD.bazel` files - Updated build rules to use centralized dependency variables - Simplified library and test suite configurations - **Library Updates** - Added Scala libraries for hub project - Updated service commons library dependencies <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: nikhil-zlai <[email protected]> Co-authored-by: Kumar Teja Chippala <[email protected]>
## Summary Migrated `hub` 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 ## Release Notes - **Dependency Management** - Added new dependency lists for Flink, Vert.x, and Circe libraries - Consolidated and standardized dependency management across multiple projects - Introduced new Maven artifact dependencies for Circe and Vert.x - **Build Configuration** - Refactored dependency references in multiple `BUILD.bazel` files - Updated build rules to use centralized dependency variables - Simplified library and test suite configurations - **Library Updates** - Added Scala libraries for hub project - Updated service commons library dependencies <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: nikhil-zlai <[email protected]> Co-authored-by: Kumar Teja Chippala <[email protected]>
## Summary Migrated `hub` 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 ## Release Notes - **Dependency Management** - Added new dependency lists for Flink, Vert.x, and Circe libraries - Consolidated and standardized dependency management across multiple projects - Introduced new Maven artifact dependencies for Circe and Vert.x - **Build Configuration** - Refactored dependency references in multiple `BUILD.bazel` files - Updated build rules to use centralized dependency variables - Simplified library and test suite configurations - **Library Updates** - Added Scala libraries for hub project - Updated service commons library dependencies <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: nikhil-zlai <[email protected]> Co-authored-by: Kumar Teja Chippala <[email protected]>
## Summary Migrated `hub` 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 ## Release Notes - **Dependency Management** - Added new dependency lists for Flink, Vert.x, and Circe libraries - Consolidated and standardized dependency management across multiple projects - Introduced new Maven artifact dependencies for Circe and Vert.x - **Build Configuration** - Refactored dependency references in multiple `BUILD.bazel` files - Updated build rules to use centralized dependency variables - Simplified library and test suite configurations - **Library Updates** - Added Scala libraries for hub project - Updated service commons library dependencies <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: nikhil-zlai <[email protected]> Co-authored-by: Kumar Teja Chippala <[email protected]>
## Summary Migrated `hub` 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 ## Release Notes - **Dependency Management** - Added new dependency lists for Flink, Vert.x, and Circe libraries - Consolidated and standardized dependency management across multiple projects - Introduced new Maven artifact dependencies for Circe and Vert.x - **Build Configuration** - Refactored dependency references in multiple `BUILD.bazel` files - Updated build rules to use centralized dependency variables - Simplified library and test suite configurations - **Library Updates** - Added Scala libraries for hub project - Updated service commons library dependencies <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: nikhil-zlai <[email protected]> Co-authored-by: Kumar Teja Chippala <[email protected]>
Summary
Migrated
hubmodule to Bazel from sbt.Checklist
Summary by CodeRabbit
Release Notes
Dependency Management
Build Configuration
BUILD.bazelfilesLibrary Updates