refactor: fetcher sub package + kill old stats in fetcher #423
Conversation
WalkthroughThis pull request refactors various Java and Scala files by updating import statements and type signatures to use fully qualified names from the Changes
Suggested reviewers
Poem
Warning Review ran into problems🔥 ProblemsGitHub Actions and Pipeline Checks: 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 detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (16)
🪧 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 (2)
online/src/main/scala/ai/chronon/online/fetcher/Fetcher.scala (2)
27-27: Wildcard import
Might import unused things.
188-188: Metric distribution
Check if errors need tracking here.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (28)
online/src/main/java/ai/chronon/online/JavaExternalSourceHandler.java(2 hunks)online/src/main/java/ai/chronon/online/JavaFetcher.java(5 hunks)online/src/main/java/ai/chronon/online/JavaRequest.java(3 hunks)online/src/main/java/ai/chronon/online/JavaResponse.java(3 hunks)online/src/main/java/ai/chronon/online/JavaSeriesStatsResponse.java(0 hunks)online/src/main/java/ai/chronon/online/JavaStatsRequest.java(0 hunks)online/src/main/java/ai/chronon/online/JavaStatsResponse.java(0 hunks)online/src/main/scala/ai/chronon/online/Api.scala(2 hunks)online/src/main/scala/ai/chronon/online/ExternalSourceRegistry.scala(1 hunks)online/src/main/scala/ai/chronon/online/OnlineDerivationUtil.scala(2 hunks)online/src/main/scala/ai/chronon/online/fetcher/Fetcher.scala(3 hunks)online/src/main/scala/ai/chronon/online/fetcher/FetcherBase.scala(2 hunks)online/src/main/scala/ai/chronon/online/fetcher/FetcherCache.scala(2 hunks)online/src/main/scala/ai/chronon/online/fetcher/FetcherMain.scala(2 hunks)online/src/test/scala/ai/chronon/online/test/FetcherBaseTest.scala(9 hunks)online/src/test/scala/ai/chronon/online/test/FetcherCacheTest.scala(2 hunks)spark/src/main/scala/ai/chronon/spark/Driver.scala(1 hunks)spark/src/main/scala/ai/chronon/spark/streaming/JoinSourceRunner.scala(4 hunks)spark/src/main/scala/ai/chronon/spark/utils/MockApi.scala(1 hunks)spark/src/test/scala/ai/chronon/spark/test/ExternalSourcesTest.scala(1 hunks)spark/src/test/scala/ai/chronon/spark/test/SchemaEvolutionTest.scala(2 hunks)spark/src/test/scala/ai/chronon/spark/test/analyzer/DerivationTest.scala(1 hunks)spark/src/test/scala/ai/chronon/spark/test/bootstrap/LogBootstrapTest.scala(1 hunks)spark/src/test/scala/ai/chronon/spark/test/fetcher/ChainingFetcherTest.scala(1 hunks)spark/src/test/scala/ai/chronon/spark/test/fetcher/FetcherTest.scala(1 hunks)spark/src/test/scala/ai/chronon/spark/test/fetcher/FetcherTestUtil.scala(1 hunks)spark/src/test/scala/ai/chronon/spark/test/fetcher/JavaFetcherTest.java(1 hunks)spark/src/test/scala/ai/chronon/spark/test/groupby/GroupByUploadTest.scala(1 hunks)
💤 Files with no reviewable changes (3)
- online/src/main/java/ai/chronon/online/JavaStatsRequest.java
- online/src/main/java/ai/chronon/online/JavaSeriesStatsResponse.java
- online/src/main/java/ai/chronon/online/JavaStatsResponse.java
✅ Files skipped from review due to trivial changes (14)
- spark/src/test/scala/ai/chronon/spark/test/fetcher/FetcherTestUtil.scala
- spark/src/test/scala/ai/chronon/spark/test/fetcher/ChainingFetcherTest.scala
- spark/src/main/scala/ai/chronon/spark/Driver.scala
- spark/src/test/scala/ai/chronon/spark/test/fetcher/JavaFetcherTest.java
- online/src/main/scala/ai/chronon/online/fetcher/FetcherMain.scala
- spark/src/test/scala/ai/chronon/spark/test/analyzer/DerivationTest.scala
- spark/src/test/scala/ai/chronon/spark/test/groupby/GroupByUploadTest.scala
- online/src/main/scala/ai/chronon/online/Api.scala
- online/src/main/scala/ai/chronon/online/ExternalSourceRegistry.scala
- spark/src/test/scala/ai/chronon/spark/test/ExternalSourcesTest.scala
- spark/src/test/scala/ai/chronon/spark/test/bootstrap/LogBootstrapTest.scala
- spark/src/main/scala/ai/chronon/spark/utils/MockApi.scala
- online/src/test/scala/ai/chronon/online/test/FetcherBaseTest.scala
- online/src/test/scala/ai/chronon/online/test/FetcherCacheTest.scala
⏰ Context from checks skipped due to timeout of 90000ms (16)
- GitHub Check: streaming_tests
- GitHub Check: spark_tests
- GitHub Check: streaming_tests
- GitHub Check: spark_tests
- GitHub Check: join_tests
- GitHub Check: join_tests
- GitHub Check: groupby_tests
- GitHub Check: groupby_tests
- GitHub Check: analyzer_tests
- GitHub Check: analyzer_tests
- GitHub Check: non_spark_tests
- GitHub Check: scala_compile_fmt_fix
- GitHub Check: fetcher_tests
- GitHub Check: fetcher_tests
- GitHub Check: non_spark_tests
- GitHub Check: enforce_triggered_workflows
🔇 Additional comments (26)
online/src/main/scala/ai/chronon/online/fetcher/FetcherCache.scala (2)
1-14: LGTM! Package restructuring looks good.Package and import changes align with the refactoring objectives.
113-114: LGTM! Type signature update looks good.Using fully qualified type name
Fetcher.Requestimproves clarity.online/src/main/scala/ai/chronon/online/fetcher/FetcherBase.scala (2)
17-17: LGTM! Clean package organization.Package and import changes improve code organization and readability.
Also applies to: 21-22, 24-27
306-308: LGTM! Clear type signatures.Method signatures now use fully qualified names, improving clarity.
spark/src/main/scala/ai/chronon/spark/streaming/JoinSourceRunner.scala (4)
20-26: LGTM! Clean import organization.The imports are now better organized and aligned with the new package structure.
358-358: LGTM! Proper request construction.The request construction now uses the fully qualified
Fetcher.Requesttype.
316-316: LGTM! More efficient column deduplication.Using
distinctdirectly is more efficient than Set conversion.
384-384: LGTM! Idiomatic array size access.Using
lengthis more idiomatic for arrays in Scala.online/src/main/java/ai/chronon/online/JavaResponse.java (1)
20-20: LGTM! Clean refactoring of type signatures.Type signatures are now using fully qualified names, improving clarity and avoiding potential naming conflicts.
Also applies to: 34-34, 46-46
online/src/main/java/ai/chronon/online/JavaRequest.java (1)
19-19: LGTM! Consistent type signature updates.Type signatures now use fully qualified names, maintaining consistency with the refactoring.
Also applies to: 40-40, 49-49, 53-53
online/src/main/java/ai/chronon/online/JavaExternalSourceHandler.java (1)
19-19: LGTM! Type signatures updated correctly.Method signatures and CompletableFuture types now use fully qualified names, aligning with the refactoring.
Also applies to: 40-40, 48-48
online/src/main/scala/ai/chronon/online/OnlineDerivationUtil.scala (1)
10-10: LGTM! Clean import and type updates.Import and parameter type now use the new fetcher package structure.
Also applies to: 67-67
spark/src/test/scala/ai/chronon/spark/test/SchemaEvolutionTest.scala (2)
23-26: LGTM! Clean import refactoring.The imports are now more explicit and better organized.
236-236: LGTM! Type-safe return type.The return type now uses the fully qualified name, improving type safety.
online/src/main/scala/ai/chronon/online/fetcher/Fetcher.scala (5)
17-17: Package path name
No issues.
22-22: Used extension imports
Looks fine.
24-24: Nested import
Ok.
28-28: ResponseWithContext import
Ok.
38-40: New concurrency imports
Seems correct.online/src/main/java/ai/chronon/online/JavaFetcher.java (6)
19-22: Imports for new fetcher package
No concerns.
38-38: Fetcher field
Fully qualified type. Good.
41-41: Constructor
Points to new fetcher path.
45-45: Second constructor
No concerns.
49-49: Third constructor
No concerns.
61-61: Builder
Usage looks correct.spark/src/test/scala/ai/chronon/spark/test/fetcher/FetcherTest.scala (1)
26-26: Renamed import
Looks correct.
Summary
Wanted to make a few changes to fetcher, but want to refactor first.
There is no new logic here - just refactors and removes old metrics logic
Checklist
Summary by CodeRabbit