-
Notifications
You must be signed in to change notification settings - Fork 0
Yank out Bigtable hbase dependency for one ApiFutureUtils class #184
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
Conversation
WalkthroughThis pull request involves removing the Google Cloud Bigtable HBase library dependency from the Changes
Sequence DiagramsequenceDiagram
participant Client
participant ApiFutureUtils
participant ApiFuture
participant CompletableFuture
Client->>ApiFutureUtils: toCompletableFuture(apiFuture)
ApiFutureUtils->>CompletableFuture: Create new instance
ApiFutureUtils->>ApiFuture: Add callback
ApiFuture-->>CompletableFuture: Complete with result/error
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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cloud_gcp/src/main/scala/ai/chronon/integrations/cloud_gcp/ApiFutureUtils.scala (1)
13-32
: Consider adding safety checks and documentation.
- Add null check for apiFuture
- Document thread safety implications of directExecutor
object ApiFutureUtils { + /** Converts Google Cloud ApiFuture to CompletableFuture. + * Note: Callbacks execute in the same thread using directExecutor. + * @throws NullPointerException if apiFuture is null + */ def toCompletableFuture[T](apiFuture: ApiFuture[T]): CompletableFuture[T] = { + if (apiFuture == null) throw new NullPointerException("apiFuture must not be null") val completableFuture: CompletableFuture[T] = new CompletableFuture[T]() {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (3)
build.sbt
(0 hunks)cloud_gcp/src/main/scala/ai/chronon/integrations/cloud_gcp/ApiFutureUtils.scala
(1 hunks)cloud_gcp/src/main/scala/ai/chronon/integrations/cloud_gcp/BigTableKVStoreImpl.scala
(0 hunks)
💤 Files with no reviewable changes (2)
- cloud_gcp/src/main/scala/ai/chronon/integrations/cloud_gcp/BigTableKVStoreImpl.scala
- build.sbt
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: table_utils_delta_format_spark_tests
- GitHub Check: other_spark_tests
- GitHub Check: fetcher_spark_tests
- GitHub Check: join_spark_tests
- GitHub Check: no_spark_scala_tests
- GitHub Check: scala_compile_fmt_fix
- GitHub Check: mutation_spark_tests
🔇 Additional comments (3)
cloud_gcp/src/main/scala/ai/chronon/integrations/cloud_gcp/ApiFutureUtils.scala (3)
1-9
: LGTM! Clean and well-organized imports.
10-11
: Clear explanation of the fork's purpose.
13-32
: Implementation looks correct.Clean conversion between future types with proper error handling.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🥳
## Summary Been working through a number of issues trying to successfully run Flink jobs on our DataProc cluster due to class / version conflicts. A number of these seem to stem from this library that brings in a lot of dependencies like hadoop ipc libs etc. We pulled in this lib for one class (ApiFutureUtils) that allows us to convert from BigTable's ApiFuture to Java's ListenableFutures. I've chosen to fork this class and yank out the lib dependency. ## Checklist - [ ] Added Unit Tests - [X] Covered by existing CI - Our bigTableKVStoreTest runs tests for the various async apis which leverage this class - [ ] Integration tested - [ ] Documentation update <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes - **Dependency Management** - Removed Google Cloud Bigtable HBase library dependency from project configuration - **New Features** - Added utility method to convert Google Cloud API futures to Java CompletableFutures - **Refactor** - Removed specific Bigtable API utility import from implementation <!-- end of auto-generated comment: release notes by coderabbit.ai --> Co-authored-by: Thomas Chow <[email protected]>
## Summary Been working through a number of issues trying to successfully run Flink jobs on our DataProc cluster due to class / version conflicts. A number of these seem to stem from this library that brings in a lot of dependencies like hadoop ipc libs etc. We pulled in this lib for one class (ApiFutureUtils) that allows us to convert from BigTable's ApiFuture to Java's ListenableFutures. I've chosen to fork this class and yank out the lib dependency. ## Checklist - [ ] Added Unit Tests - [X] Covered by existing CI - Our bigTableKVStoreTest runs tests for the various async apis which leverage this class - [ ] Integration tested - [ ] Documentation update <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes - **Dependency Management** - Removed Google Cloud Bigtable HBase library dependency from project configuration - **New Features** - Added utility method to convert Google Cloud API futures to Java CompletableFutures - **Refactor** - Removed specific Bigtable API utility import from implementation <!-- end of auto-generated comment: release notes by coderabbit.ai -->
## Summary Been working through a number of issues trying to successfully run Flink jobs on our DataProc cluster due to class / version conflicts. A number of these seem to stem from this library that brings in a lot of dependencies like hadoop ipc libs etc. We pulled in this lib for one class (ApiFutureUtils) that allows us to convert from BigTable's ApiFuture to Java's ListenableFutures. I've chosen to fork this class and yank out the lib dependency. ## Checklist - [ ] Added Unit Tests - [X] Covered by existing CI - Our bigTableKVStoreTest runs tests for the various async apis which leverage this class - [ ] Integration tested - [ ] Documentation update <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes - **Dependency Management** - Removed Google Cloud Bigtable HBase library dependency from project configuration - **New Features** - Added utility method to convert Google Cloud API futures to Java CompletableFutures - **Refactor** - Removed specific Bigtable API utility import from implementation <!-- end of auto-generated comment: release notes by coderabbit.ai -->
## Summary Been working through a number of issues trying to successfully run Flink jobs on our DataProc cluster due to class / version conflicts. A number of these seem to stem from this library that brings in a lot of dependencies like hadoop ipc libs etc. We pulled in this lib for one class (ApiFutureUtils) that allows us to convert from BigTable's ApiFuture to Java's ListenableFutures. I've chosen to fork this class and yank out the lib dependency. ## Checklist - [ ] Added Unit Tests - [X] Covered by existing CI - Our bigTableKVStoreTest runs tests for the various async apis which leverage this class - [ ] Integration tested - [ ] Documentation update <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes - **Dependency Management** - Removed Google Cloud Bigtable HBase library dependency from project configuration - **New Features** - Added utility method to convert Google Cloud API futures to Java CompletableFutures - **Refactor** - Removed specific Bigtable API utility import from implementation <!-- end of auto-generated comment: release notes by coderabbit.ai -->
## Summary Been working through a number of issues trying to successfully run Flink jobs on our DataProc cluster due to class / version conflicts. A number of these seem to stem from this library that brings in a lot of dependencies like hadoop ipc libs etc. We pulled in this lib for one class (ApiFutureUtils) that allows us to convert from BigTable's ApiFuture to Java's ListenableFutures. I've chosen to fork this class and yank out the lib dependency. ## Cheour clientslist - [ ] Added Unit Tests - [X] Covered by existing CI - Our bigTableKVStoreTest runs tests for the various async apis which leverage this class - [ ] Integration tested - [ ] Documentation update <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes - **Dependency Management** - Removed Google Cloud Bigtable HBase library dependency from project configuration - **New Features** - Added utility method to convert Google Cloud API futures to Java CompletableFutures - **Refactor** - Removed specific Bigtable API utility import from implementation <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Summary
Been working through a number of issues trying to successfully run Flink jobs on our DataProc cluster due to class / version conflicts. A number of these seem to stem from this library that brings in a lot of dependencies like hadoop ipc libs etc. We pulled in this lib for one class (ApiFutureUtils) that allows us to convert from BigTable's ApiFuture to Java's ListenableFutures. I've chosen to fork this class and yank out the lib dependency.
Checklist
Summary by CodeRabbit
Release Notes
Dependency Management
New Features
Refactor