Adding kafka offset support for period and timestamp by ravishankar15 · Pull Request #9193 · apache/pinot

ravishankar15 · 2022-08-10T18:43:28Z

codecov-commenter · 2022-08-11T04:25:58Z

Codecov Report

Merging #9193 (4afafc9) into master (a4e3adb) will decrease coverage by 45.21%.
The diff coverage is 3.57%.

❗ Current head 4afafc9 differs from pull request most recent head 79535ee. Consider uploading reports for the commit 79535ee to get more accurate results

@@              Coverage Diff              @@
##             master    #9193       +/-   ##
=============================================
- Coverage     69.98%   24.76%   -45.22%     
+ Complexity     5007       53     -4954     
=============================================
  Files          1854     1843       -11     
  Lines         98871    98572      -299     
  Branches      15043    15015       -28     
=============================================
- Hits          69191    24415    -44776     
- Misses        24799    71725    +46926     
+ Partials       4881     2432     -2449

Flag	Coverage Δ
integration1	`?`
integration2	`24.76% <3.57%> (-0.06%)`	⬇️
unittests1	`?`
unittests2	`?`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...ry/rules/PinotLogicalSortFetchEliminationRule.java	`0.00% <0.00%> (ø)`
...g/apache/pinot/query/rules/PinotQueryRuleSets.java	`0.00% <ø> (-100.00%)`	⬇️
...va/org/apache/pinot/spi/stream/OffsetCriteria.java	`0.00% <0.00%> (-90.00%)`	⬇️
...ain/java/org/apache/pinot/spi/utils/TimeUtils.java	`0.00% <0.00%> (-97.44%)`	⬇️
...in/stream/kafka20/KafkaStreamMetadataProvider.java	`40.00% <5.55%> (-31.43%)`	⬇️
...ava/org/apache/pinot/core/plan/FilterPlanNode.java	`56.39% <50.00%> (-26.06%)`	⬇️
...in/java/org/apache/pinot/spi/utils/BytesUtils.java	`0.00% <0.00%> (-100.00%)`	⬇️
.../java/org/apache/pinot/spi/utils/BooleanUtils.java	`0.00% <0.00%> (-100.00%)`	⬇️
...java/org/apache/pinot/spi/trace/BaseRecording.java	`0.00% <0.00%> (-100.00%)`	⬇️
...java/org/apache/pinot/spi/trace/NoOpRecording.java	`0.00% <0.00%> (-100.00%)`	⬇️
... and 1359 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Jackie-Jiang

Mostly good!

Jackie-Jiang · 2022-08-15T22:26:27Z

pinot-spi/src/main/java/org/apache/pinot/spi/stream/OffsetCriteria.java

Modify the exception message

Jackie-Jiang · 2022-08-15T22:27:07Z

pinot-spi/src/main/java/org/apache/pinot/spi/utils/TimeUtils.java

Seems we want to go back a certain period from the passed in time. IMO this method is a little bit confusing, so suggest removing it and make the caller use convertPeriodToMillis() instead

Jackie-Jiang · 2022-08-15T22:52:35Z

pinot-spi/src/main/java/org/apache/pinot/spi/utils/TimeUtils.java

Suggest not adding this extra check. Negative period should also be valid

Jackie-Jiang · 2022-08-15T22:53:13Z

pinot-spi/src/main/java/org/apache/pinot/spi/utils/TimeUtils.java

Suggested change

public static Long convertDateTimeToMillis(String timeStr) {

public static Long convertTimestampToMillis(String timeStr) {

npawar · 2022-08-16T16:58:32Z

...ka-2.0/src/main/java/org/apache/pinot/plugin/stream/kafka20/KafkaStreamMetadataProvider.java

nit: in both cases, log a warn if offsetAndTimestamp == null?

Jackie-Jiang

LGTM otherwise

Jackie-Jiang · 2022-08-16T22:57:12Z

pinot-spi/src/main/java/org/apache/pinot/spi/stream/OffsetCriteria.java

Since we already check if the period is valid, we should just remove the try-catch

Jackie-Jiang · 2022-08-16T22:57:57Z

...ka-2.0/src/main/java/org/apache/pinot/plugin/stream/kafka20/KafkaStreamMetadataProvider.java

Suggest also logging the topic partition and offset string to provide more info for debugging

Jackie-Jiang · 2022-08-16T22:58:04Z

...ka-2.0/src/main/java/org/apache/pinot/plugin/stream/kafka20/KafkaStreamMetadataProvider.java

Add date format to comment Fixing the review comments Add warn logs for better observability Remove the try catch and enhance the log statement Fix checkstyle

Jackie-Jiang · 2022-08-17T18:15:14Z

@ravishankar15 Thanks for contributing the feature! Can you help also update the Pinot documentation to include the new config options? The documentation is backed by this repo: https://github.com/pinot-contrib/pinot-docs

npawar · 2022-08-27T22:29:16Z

Fix #7250

@ravishankar15 would you please update the docs? Right now it reads incorrect: https://docs.pinot.apache.org/basics/data-import/pinot-stream-ingestion/import-from-apache-kafka#creating-a-table-configuration

Here as well: https://docs.pinot.apache.org/configuration-reference/table#indexing-config

Thanks in advance!

ravishankar15 · 2022-08-28T13:19:37Z

Fix #7250

@ravishankar15 would you please update the docs? Right now it reads incorrect: https://docs.pinot.apache.org/basics/data-import/pinot-stream-ingestion/import-from-apache-kafka#creating-a-table-configuration

Here as well: https://docs.pinot.apache.org/configuration-reference/table#indexing-config

Thanks in advance!

@npawar I think the master changes in the doc is not reflecting in the official documentation I have made the doc changes to the master Ref: pinot-contrib/pinot-docs#115

Jackie-Jiang reviewed Aug 15, 2022

View reviewed changes

ravishankar15 force-pushed the kafka-offset branch 2 times, most recently from 6d54cb0 to 680b8f7 Compare August 16, 2022 16:16

npawar reviewed Aug 16, 2022

View reviewed changes

ravishankar15 force-pushed the kafka-offset branch from d0df255 to 79535ee Compare August 16, 2022 18:04

Jackie-Jiang approved these changes Aug 16, 2022

View reviewed changes

ravishankar15 force-pushed the kafka-offset branch 2 times, most recently from 22a4392 to 20e0d74 Compare August 17, 2022 11:03

Adding kafka offset support for period and timestamp

59f4860

Add date format to comment Fixing the review comments Add warn logs for better observability Remove the try catch and enhance the log statement Fix checkstyle

ravishankar15 force-pushed the kafka-offset branch from 20e0d74 to 59f4860 Compare August 17, 2022 11:53

Jackie-Jiang merged commit c1bdd10 into apache:master Aug 17, 2022

Jackie-Jiang added feature release-notes Referenced by PRs that need attention when compiling the next release notes labels Aug 17, 2022

	public static Long convertDateTimeToMillis(String timeStr) {
	public static Long convertTimestampToMillis(String timeStr) {

Conversation

ravishankar15 commented Aug 10, 2022

Uh oh!

codecov-commenter commented Aug 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Jackie-Jiang left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jackie-Jiang left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jackie-Jiang commented Aug 17, 2022

Uh oh!

npawar commented Aug 27, 2022

Uh oh!

ravishankar15 commented Aug 28, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov-commenter commented Aug 11, 2022 •

edited

Loading