Allow setting custom time boundary for hybrid table queries #9356

saurabhd336 · 2022-09-09T15:44:39Z

#9332

New added controller rest API:

POST tables/{tableName}/timeBoundary: Set hybrid table query time boundary based on offline segments' metadata (max end time)
DELETE tables/{tableName}/timeBoundary: Remove the time boundary

New added server rest API:

GET tables/{tableNameWithType}/allSegmentsLoaded: Validates if the ideal state matches with the segment state on the server

codecov-commenter · 2022-09-19T06:58:37Z

Codecov Report

Merging #9356 (9fc8787) into master (421154f) will decrease coverage by 0.04%.
The diff coverage is 30.89%.

@@             Coverage Diff              @@
##             master    #9356      +/-   ##
============================================
- Coverage     69.96%   69.92%   -0.05%     
- Complexity     5197     5202       +5     
============================================
  Files          1924     1927       +3     
  Lines        102414   102693     +279     
  Branches      15539    15586      +47     
============================================
+ Hits          71655    71806     +151     
- Misses        25695    25830     +135     
+ Partials       5064     5057       -7

Flag	Coverage Δ
integration1	`26.03% <23.57%> (+0.05%)`	⬆️
integration2	`24.64% <23.57%> (+0.04%)`	⬆️
unittests1	`67.23% <25.00%> (-0.02%)`	⬇️
unittests2	`15.57% <32.29%> (-0.02%)`	⬇️

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

Impacted Files	Coverage Δ
.../restlet/resources/TableSegmentValidationInfo.java	`0.00% <0.00%> (ø)`
...che/pinot/server/api/resources/TablesResource.java	`38.88% <0.00%> (-5.10%)`	⬇️
...va/org/apache/pinot/spi/utils/CommonConstants.java	`24.32% <ø> (-0.68%)`	⬇️
...oller/api/resources/PinotTableRestletResource.java	`55.17% <5.76%> (-8.25%)`	⬇️
...rg/apache/pinot/server/starter/ServerInstance.java	`85.81% <50.00%> (-0.52%)`	⬇️
...oker/routing/timeboundary/TimeBoundaryManager.java	`91.83% <88.57%> (+0.47%)`	⬆️
...che/pinot/broker/routing/BrokerRoutingManager.java	`86.07% <100.00%> (ø)`
...a/org/apache/pinot/common/metrics/BrokerGauge.java	`94.11% <100.00%> (+0.36%)`	⬆️
.../pinot/common/function/scalar/ObjectFunctions.java	`66.66% <0.00%> (-20.84%)`	⬇️
...ion/function/DistinctCountAggregationFunction.java	`50.22% <0.00%> (-8.53%)`	⬇️
... and 65 more

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

Jackie-Jiang · 2022-09-21T00:45:08Z

...oker/src/test/java/org/apache/pinot/broker/routing/timeboundary/TimeBoundaryManagerTest.java

Do we need to change this?

Jackie-Jiang · 2022-09-21T00:48:43Z

...t-broker/src/main/java/org/apache/pinot/broker/routing/timeboundary/TimeBoundaryManager.java

(minor) We shouldn't need to keep them separate. We can directly update the _timeBoundaryInfo when the time is set in IS

I figured some tricky edge cases with that which could lead to wrong time boundaries being used especially during cases like concurrent segment refreshes along with enforced time boundary being set / unset. This felt like the safest..
We can discuss this offline

...t-broker/src/main/java/org/apache/pinot/broker/routing/timeboundary/TimeBoundaryManager.java

Jackie-Jiang · 2022-09-21T00:52:01Z

...t-broker/src/main/java/org/apache/pinot/broker/routing/timeboundary/TimeBoundaryManager.java

Same here. When the time boundary is explicitly set, we want to skip the _endTimeMsMap updates. Same for refreshSegment()

It is needed to correctly update the metric that tracks the difference b/w current time boundary and max(end time across all segments)

Jackie-Jiang · 2022-09-21T00:56:29Z

pinot-server/src/main/java/org/apache/pinot/server/starter/helix/AdminApiApplication.java

(minor) We can pass HelixManager through the ServerInstance. We already pass it in the ServerInstance constructor

Jackie-Jiang · 2022-09-21T01:01:19Z

...oller/src/main/java/org/apache/pinot/controller/api/resources/PinotTableRestletResource.java

Suggest simplifying it. Same for the DELETE API

Suggested change

@Path("table/{tableName}/enforceQueryTimeBoundary")

@Path("table/{tableName}/timeBoundary")

Jackie-Jiang · 2022-09-21T01:02:57Z

...oller/src/main/java/org/apache/pinot/controller/api/resources/PinotTableRestletResource.java

The input has to be raw table name, or the table validation will fail. Consider revising the parameter description to be more explicit

Jackie-Jiang · 2022-09-21T02:21:44Z

pinot-server/src/main/java/org/apache/pinot/server/api/resources/TablesResource.java

We want to check the state of the server in IS. For ONLINE, check the CRC; For CONSUMING, check if the segment exist; Skip other states.

Jackie-Jiang · 2022-09-21T02:24:29Z

pinot-server/src/main/java/org/apache/pinot/server/api/resources/TablesResource.java

Suggest more explicit API, such as /allSegmentsLoaded

Jackie-Jiang · 2022-09-21T02:27:42Z

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

(optional) I feel TIME_BOUNDARY is more concise. To be more specific, HYBRID_TABLE_TIME_BOUNDARY might also be good

Jackie-Jiang

Mostly minor comments

Jackie-Jiang · 2022-09-30T21:27:04Z

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

Suggested change

public static final String QUERY_TIME_BOUNDARY = "HYBRID_TABLE_TIME_BOUNDARY";

public static final String HYBRID_TABLE_TIME_BOUNDARY = "HYBRID_TABLE_TIME_BOUNDARY";

Jackie-Jiang · 2022-09-30T21:27:22Z

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

(format) shouldn't remove

Jackie-Jiang · 2022-09-30T21:31:36Z

pinot-server/src/main/java/org/apache/pinot/server/api/resources/TablesResource.java

(minor)

Suggested change

@ApiOperation(value = "Validates if the ideal state matches with the segmentstate on this server", notes =

"Validates if the ideal state matches with the segmentstate on this server")

@ApiOperation(value = "Validates if the ideal state matches with the segment state on this server", notes =

"Validates if the ideal state matches with the segment state on this server")

Jackie-Jiang · 2022-09-30T21:36:10Z

pinot-server/src/main/java/org/apache/pinot/server/api/resources/TablesResource.java

Consider throwing proper exception when zkMetadata is null, or it will get NPE here

Jackie-Jiang · 2022-09-30T21:44:21Z

...oller/src/main/java/org/apache/pinot/controller/api/resources/PinotTableRestletResource.java

Suggested change

@Path("table/{tableName}/timeBoundary")

@Path("tables/{tableName}/timeBoundary")

Jackie-Jiang · 2022-09-30T22:15:32Z

...t-broker/src/main/java/org/apache/pinot/broker/routing/timeboundary/TimeBoundaryManager.java

(minor) We don't need to synchronize on it because it can only be called in the synchronized method

Jackie-Jiang · 2022-09-30T22:19:50Z

...t-broker/src/main/java/org/apache/pinot/broker/routing/timeboundary/TimeBoundaryManager.java

We always want to update the metrics. Currently when user refresh a segment, the metric won't be updated.

Good catch! Ack

Jackie-Jiang · 2022-09-30T22:20:47Z

...t-broker/src/main/java/org/apache/pinot/broker/routing/timeboundary/TimeBoundaryManager.java

Here we also want to check if the explicitly set flag is the same

Jackie-Jiang · 2022-09-30T22:22:13Z

...t-broker/src/main/java/org/apache/pinot/broker/routing/timeboundary/TimeBoundaryManager.java

(minor) Consider following the same convention of using -1 to represent the invalid value

Jackie-Jiang · 2022-09-30T22:31:58Z

...t-broker/src/main/java/org/apache/pinot/broker/routing/timeboundary/TimeBoundaryManager.java

This part is quite complicated. It will be good if we can simplify the logic, or put some comments explaining the logic under each if branch

I couldn't seem to find a way to simplify this without using 2 different TimeBoundaryInfo instances for explicitly set and derived time boundaries. Have added comments for now.

saurabhd336 · 2022-10-04T05:12:10Z

...t-broker/src/main/java/org/apache/pinot/broker/routing/timeboundary/TimeBoundaryManager.java

  private final long _timeOffsetMs;
  private final Map<String, Long> _endTimeMsMap = new HashMap<>();

+  private long _explicitlySetTimeBoundaryMs = INVALID_TIME_MS;


Should be marked @volatile

Doesn't need to be because it is always under the synchronized block. Maybe worth adding a comment

saurabhd336 changed the title ~~Allow setting custom time boundary for hybrid table queries~~ Allow setting custom time boundary for hybrid table queries (WIP) Sep 9, 2022

saurabhd336 force-pushed the customTimeBoundary branch 3 times, most recently from 1f7f579 to d16f9cc Compare September 12, 2022 06:32

saurabhd336 force-pushed the customTimeBoundary branch from d16f9cc to 6a45644 Compare September 19, 2022 06:03

saurabhd336 changed the title ~~Allow setting custom time boundary for hybrid table queries (WIP)~~ Allow setting custom time boundary for hybrid table queries Sep 19, 2022

saurabhd336 force-pushed the customTimeBoundary branch 2 times, most recently from ab772c7 to 4fb86e6 Compare September 19, 2022 10:36

Jackie-Jiang reviewed Sep 21, 2022

View reviewed changes

saurabhd336 force-pushed the customTimeBoundary branch 5 times, most recently from b2711c5 to 0480ea1 Compare September 30, 2022 06:09

Jackie-Jiang reviewed Sep 30, 2022

View reviewed changes

Saurabh Dubey added 9 commits October 3, 2022 11:05

Add controller API to enforce hybrid table time boundary

9f7c2a4

Add delete API

382fa47

Fix timebounday null cases

41a6ac2

Fix lint

bad91e1

Fix lint + bindings

7793971

Fix tests

34a4553

Review comments

bae34ad

Review comments

597636d

Review comments

0d92260

saurabhd336 force-pushed the customTimeBoundary branch from a93441e to 0d92260 Compare October 3, 2022 05:35

saurabhd336 requested a review from Jackie-Jiang October 3, 2022 10:12

Simplify time boundary handling

9fc8787

Jackie-Jiang force-pushed the customTimeBoundary branch from 414eaf6 to 9fc8787 Compare October 4, 2022 05:03

saurabhd336 commented Oct 4, 2022

View reviewed changes

Jackie-Jiang approved these changes Oct 4, 2022

View reviewed changes

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

Jackie-Jiang merged commit 79c27e0 into apache:master Oct 4, 2022

	@Path("table/{tableName}/enforceQueryTimeBoundary")
	@Path("table/{tableName}/timeBoundary")

	public static final String QUERY_TIME_BOUNDARY = "HYBRID_TABLE_TIME_BOUNDARY";
	public static final String HYBRID_TABLE_TIME_BOUNDARY = "HYBRID_TABLE_TIME_BOUNDARY";

-  @ApiOperation(value = "Validates if the ideal state matches with the segmentstate on this server", notes =
-      "Validates if the ideal state matches with the segmentstate on this server")
+  @ApiOperation(value = "Validates if the ideal state matches with the segment state on this server", notes =
+      "Validates if the ideal state matches with the segment state on this server")

	@Path("table/{tableName}/timeBoundary")
	@Path("tables/{tableName}/timeBoundary")

Allow setting custom time boundary for hybrid table queries #9356

Allow setting custom time boundary for hybrid table queries #9356

Uh oh!

Conversation

saurabhd336 commented Sep 9, 2022 • edited by Jackie-Jiang Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Codecov Report

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

saurabhd336 Sep 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

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!

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!

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

saurabhd336 commented Sep 9, 2022 •

edited by Jackie-Jiang

Loading

codecov-commenter commented Sep 19, 2022 •

edited

Loading

saurabhd336 Sep 30, 2022 •

edited

Loading