OSS909 -- Make fetcher thread pool separate and pluggable #282

varant-zlai · 2025-01-28T00:53:47Z

Summary

Manual cherry-pick from https://github.com/airbnb/chronon/pull/909/files

Should be rebased and tested off of #276

Checklist

Summary by CodeRabbit

New Features
- Added a builder pattern for JavaFetcher to enhance object creation flexibility.
- Introduced an optional ExecutionContext parameter across multiple classes to improve execution context management.
Improvements
- Updated constructor signatures in Java and Scala classes to support more configurable execution contexts.
- Enhanced instantiation process with more structured object creation methods.

coderabbitai · 2025-01-28T00:53:54Z

Walkthrough

The changes introduce an optional executionContextOverride parameter across multiple classes in the online package, specifically JavaFetcher, Fetcher, FetcherBase, and MetadataStore. This modification allows for more flexible execution context management by enabling developers to inject a custom ExecutionContext during object initialization. The changes are primarily focused on constructor signatures and include the addition of a builder pattern for JavaFetcher.

Changes

File	Changes
`online/src/main/java/ai/chronon/online/JavaFetcher.java`	- Added builder pattern with `Builder` class - Added `executionContextOverride` to three constructors - Added builder methods for configuration
`online/src/main/scala/ai/chronon/online/Fetcher.scala`	- Added `executionContextOverride` parameter to constructor - Updated `FetcherBase` constructor call - Added `ExecutionContext` import
`online/src/main/scala/ai/chronon/online/FetcherBase.scala`	- Added `executionContextOverride` parameter to constructor - Updated `MetadataStore` constructor call
`online/src/main/scala/ai/chronon/online/MetadataStore.scala`	- Added `executionContextOverride` parameter to constructor - Modified execution context determination logic

Sequence Diagram

sequenceDiagram
    participant Developer
    participant JavaFetcher.Builder
    participant JavaFetcher
    
    Developer->>JavaFetcher.Builder: Create builder
    Developer->>JavaFetcher.Builder: Configure with methods
    JavaFetcher.Builder->>JavaFetcher: build()
    JavaFetcher-->>Developer: Return configured JavaFetcher

Poem

🚀 Contexts dancing free and light,
Builders crafting with delight,
Execution paths now more precise,
Flexibility wrapped in code so nice,
A symphony of threads take flight! 🧵

Warning

Review ran into problems

🔥 Problems

GitHub 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.

📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)

📥 Commits

Reviewing files that changed from the base of the PR and between b25cbf3 and 276332a.

📒 Files selected for processing (1)

online/src/main/java/ai/chronon/online/JavaFetcher.java (2 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

online/src/main/java/ai/chronon/online/JavaFetcher.java

⏰ Context from checks skipped due to timeout of 90000ms (2)

GitHub Check: scala_compile_fmt_fix
GitHub Check: no_spark_scala_tests

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

Review comments: Directly reply to a review comment made by CodeRabbit. Example:
- I pushed a fix in commit <commit_id>, please review it.
- Generate unit testing code for this file.
- Open a follow-up GitHub issue for this discussion.
Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
- @coderabbitai generate unit testing code for this file.
- @coderabbitai modularize this function.
PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
- @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
- @coderabbitai read src/utils.ts and generate unit testing code.
- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
- @coderabbitai help me debug CodeRabbit configuration file.

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)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai full review to do a full review from scratch and review all the files again.
@coderabbitai summary to regenerate the summary of the PR.
@coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai configuration to show the current CodeRabbit configuration for the repository.
@coderabbitai help to get help.

Other keywords and placeholders

Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (`.coderabbit.yaml`)

You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
Please see the configuration documentation for more information.
If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

Visit our Documentation for detailed information on how to use CodeRabbit.
Join our Discord Community to get help, request features, and share feedback.
Follow us on X/Twitter for updates and announcements.

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (3)

online/src/main/java/ai/chronon/online/JavaFetcher.java (3)
39-47: Consider defining a constant for the default executionContext.

All constructors pass null for executionContextOverride. Define a private static final field for better maintainability.
+ private static final ExecutionContext DEFAULT_EXECUTION_CONTEXT = null;

  public JavaFetcher(...) {
-   this.fetcher = new Fetcher(..., null);
+   this.fetcher = new Fetcher(..., DEFAULT_EXECUTION_CONTEXT);
  }
50-57: Update example to show executionContextOverride usage.

71-81: Initialize boolean fields with defaults.

Prevent potential NPEs by initializing optional boolean fields.
  public static class Builder {
    private Boolean debug = false;
    private Boolean disableErrorThrows = false;

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)

📥 Commits

Reviewing files that changed from the base of the PR and between 9a2bf3a and b25cbf3.

📒 Files selected for processing (4)

online/src/main/java/ai/chronon/online/JavaFetcher.java (2 hunks)
online/src/main/scala/ai/chronon/online/Fetcher.scala (2 hunks)
online/src/main/scala/ai/chronon/online/FetcherBase.scala (2 hunks)
online/src/main/scala/ai/chronon/online/MetadataStore.scala (2 hunks)

🚧 Files skipped from review as they are similar to previous changes (3)

online/src/main/scala/ai/chronon/online/MetadataStore.scala
online/src/main/scala/ai/chronon/online/Fetcher.scala
online/src/main/scala/ai/chronon/online/FetcherBase.scala

⏰ Context from checks skipped due to timeout of 90000ms (2)

GitHub Check: scala_compile_fmt_fix
GitHub Check: no_spark_scala_tests

🔇 Additional comments (2)

online/src/main/java/ai/chronon/online/JavaFetcher.java (2)

27-27: LGTM: Import added for ExecutionContext.

83-115: LGTM: Builder implementation follows best practices.

Clean implementation with proper method chaining.

online/src/main/java/ai/chronon/online/JavaFetcher.java

nikhil-zlai · 2025-01-28T18:00:01Z

online/src/main/java/ai/chronon/online/JavaFetcher.java

+            builder.executionContextOverride);
+  }
+
+  public static class Builder {


very nice!

very enterprisey :-)

Co-authored-by: Nikhil Simha <[email protected]>

## Summary Manual cherry-pick from https://github.com/airbnb/chronon/pull/909/files Should be rebased and tested off of #276 ## Checklist - [ ] Added Unit Tests - [ ] Covered by existing CI - [ ] Integration tested - [ ] Documentation update - [x] Not yet tested, should test after merging 276  ## Summary by CodeRabbit - **New Features** - Added a builder pattern for `JavaFetcher` to enhance object creation flexibility. - Introduced an optional `ExecutionContext` parameter across multiple classes to improve execution context management. - **Improvements** - Updated constructor signatures in Java and Scala classes to support more configurable execution contexts. - Enhanced instantiation process with more structured object creation methods.  --------- Co-authored-by: ezvz <[email protected]> Co-authored-by: Nikhil Simha <[email protected]> Co-authored-by: Thomas Chow <[email protected]>

## Summary Manual cherry-piour clients from https://github.com/airbnb/chronon/pull/909/files Should be rebased and tested off of #276 ## Cheour clientslist - [ ] Added Unit Tests - [ ] Covered by existing CI - [ ] Integration tested - [ ] Documentation update - [x] Not yet tested, should test after merging 276  ## Summary by CodeRabbit - **New Features** - Added a builder pattern for `JavaFetcher` to enhance object creation flexibility. - Introduced an optional `ExecutionContext` parameter across multiple classes to improve execution context management. - **Improvements** - Updated constructor signatures in Java and Scala classes to support more configurable execution contexts. - Enhanced instantiation process with more structured object creation methods.  --------- Co-authored-by: ezvz <[email protected]> Co-authored-by: Nikhil Simha <[email protected]> Co-authored-by: Thomas Chow <[email protected]>

varant-zlai requested a review from nikhil-zlai January 28, 2025 00:53

WIP

b25cbf3

varant-zlai force-pushed the vz--cherry_pick_oss_909_thread_pool_seperate branch from 9a2bf3a to b25cbf3 Compare January 28, 2025 15:50

coderabbitai bot reviewed Jan 28, 2025

View reviewed changes

nikhil-zlai reviewed Jan 28, 2025

View reviewed changes

online/src/main/java/ai/chronon/online/JavaFetcher.java Outdated Show resolved Hide resolved

nikhil-zlai reviewed Jan 28, 2025

View reviewed changes

nikhil-zlai previously approved these changes Jan 28, 2025

View reviewed changes

Update online/src/main/java/ai/chronon/online/JavaFetcher.java

276332a

Co-authored-by: Nikhil Simha <[email protected]>

varant-zlai dismissed nikhil-zlai’s stale review via 276332a January 28, 2025 19:26

Merge branch 'main' into vz--cherry_pick_oss_909_thread_pool_seperate

91300ee

tchow-zlai approved these changes Jan 28, 2025

View reviewed changes

varant-zlai merged commit 5c5d6aa into main Jan 28, 2025
6 checks passed

varant-zlai deleted the vz--cherry_pick_oss_909_thread_pool_seperate branch January 28, 2025 20:46

This was referenced Feb 24, 2025

refactor: split fetcher logic into multiple files #425

Merged

Enable batch IR caching by default + fix vertx bt startup issue #435

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

OSS909 -- Make fetcher thread pool separate and pluggable #282

OSS909 -- Make fetcher thread pool separate and pluggable #282

Uh oh!

varant-zlai commented Jan 28, 2025 •

edited by coderabbitai bot

Loading

Uh oh!

coderabbitai bot commented Jan 28, 2025 •

edited

Loading

Review ran into problems

Chat

CodeRabbit Commands (Invoked using PR comments)

Other keywords and placeholders

CodeRabbit Configuration File (`.coderabbit.yaml`)

Documentation and Community

Uh oh!

coderabbitai bot left a comment

Uh oh!

Uh oh!

nikhil-zlai Jan 28, 2025

Uh oh!

Uh oh!

Uh oh!

OSS909 -- Make fetcher thread pool separate and pluggable #282

OSS909 -- Make fetcher thread pool separate and pluggable #282

Uh oh!

Conversation

varant-zlai commented Jan 28, 2025 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

Summary by CodeRabbit

Uh oh!

coderabbitai bot commented Jan 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram

Poem

Review ran into problems

Chat

CodeRabbit Commands (Invoked using PR comments)

Other keywords and placeholders

CodeRabbit Configuration File (.coderabbit.yaml)

Documentation and Community

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nikhil-zlai Jan 28, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

varant-zlai commented Jan 28, 2025 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Jan 28, 2025 •

edited

Loading

CodeRabbit Configuration File (`.coderabbit.yaml`)