Turn on v2 engine by default #10543

xiangfu0 · 2023-04-04T21:37:30Z

Turn on v2 engine by default.

Make v2 engine enabled by default
Use dynamic port for broker mailbox port and server mailbox & query runner ports if not specified. This requires updating helix instanceConfig during initialization.
Bump default DataTable version to 4.

Also tested the broker/server restart with dynamic port changes, reflected in helix instance configs.

siddharthteotia · 2023-04-04T21:41:53Z

IIRC - default enabling the feature flag will have no impact on existing production queries unless they are also changed to include the query option to use the multi stage query engine ?

xiangfu0 · 2023-04-04T21:55:06Z

IIRC - default enabling the feature flag will have no impact on existing production queries unless they are also changed to include the query option to use the multi stage query engine ?

This is just to start the query engine by default, ofc we will see extra resources utilization (grpc server started/ports open etc).

The normal query path won't go through it unless specifying useMultistageEngine=true in the query option.

walterddr · 2023-04-04T22:49:48Z

...est/java/org/apache/pinot/integration/tests/MultiStageEngineCustomTenantIntegrationTest.java

@@ -139,13 +138,11 @@ protected Connection getPinotConnection() {

  @Override
  protected void overrideBrokerConf(PinotConfiguration brokerConf) {
-    brokerConf.setProperty(CommonConstants.Helix.CONFIG_OF_MULTI_STAGE_ENGINE_ENABLED, true);
    brokerConf.setProperty(QueryConfig.KEY_OF_QUERY_RUNNER_PORT, 8421);


so we still need these port config? can we check whether it is possible to default dynamic port allocation?

Those shouldn't matter, let me remove them as well

The issue is instance config in helix is using port 0, so workers cannot talk to each other. Need to find the available port first then set it into server config.

codecov-commenter · 2023-04-04T23:04:00Z

Codecov Report

Merging #10543 (a8b222c) into master (f6c6d14) will decrease coverage by 0.01%.
The diff coverage is 53.57%.

@@             Coverage Diff              @@
##             master   #10543      +/-   ##
============================================
- Coverage     70.35%   70.35%   -0.01%     
  Complexity     6464     6464              
============================================
  Files          2103     2103              
  Lines        112769   112792      +23     
  Branches      16981    16988       +7     
============================================
+ Hits          79341    79353      +12     
+ Misses        27877    27863      -14     
- Partials       5551     5576      +25

Flag	Coverage Δ
integration1	`24.55% <53.57%> (+0.09%)`	⬆️
integration2	`24.00% <53.57%> (-0.25%)`	⬇️
unittests1	`67.84% <100.00%> (+<0.01%)`	⬆️
unittests2	`13.89% <45.83%> (+0.02%)`	⬆️

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

Impacted Files	Coverage Δ
...core/common/datatable/DataTableBuilderFactory.java	`58.82% <ø> (ø)`
...va/org/apache/pinot/spi/utils/CommonConstants.java	`24.39% <ø> (ø)`
...e/pinot/broker/broker/helix/BaseBrokerStarter.java	`76.19% <47.61%> (-3.04%)`	⬇️
.../pinot/server/starter/helix/BaseServerStarter.java	`56.79% <50.00%> (-0.56%)`	⬇️
...requesthandler/MultiStageBrokerRequestHandler.java	`69.79% <100.00%> (+1.04%)`	⬆️
...apache/pinot/common/datatable/DataTableImplV4.java	`91.10% <100.00%> (+0.99%)`	⬆️

... and 31 files with indirect coverage changes

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

siddharthteotia · 2023-04-05T13:41:44Z

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

+      _serverConf.setProperty(QueryConfig.KEY_OF_QUERY_SERVER_PORT, NetUtils.findOpenPort());
+    }
+    if (_serverConf.getProperty(QueryConfig.KEY_OF_QUERY_RUNNER_PORT, QueryConfig.DEFAULT_QUERY_RUNNER_PORT) == 0) {
+      _serverConf.setProperty(QueryConfig.KEY_OF_QUERY_RUNNER_PORT, NetUtils.findOpenPort());


Doing this can have query runner port differ from one worker to another ? In that case, how will they talk to each other ?

port configurations are stored in serverInstance and it is backed up to Helix via InstanceConfig

It comes from the instanceConfig in Helix.

ankitsultana · 2023-04-05T17:42:55Z

pinot-tools/src/main/java/org/apache/pinot/tools/MultistageEngineQuickStart.java

@@ -94,7 +94,6 @@ public String[] getDefaultBatchTableDirectories() {
  @Override
  public Map<String, Object> getConfigOverrides() {
    Map<String, Object> overrides = new HashMap<>(super.getConfigOverrides());
-    overrides.put("pinot.multistage.engine.enabled", "true");
    overrides.put("pinot.server.instance.currentDataTableVersion", 4);


By default data table version is 3. I think enabling multistage engine alone wouldn't be sufficient since v2 queries will fail with data-table v3.

pinot/pinot-core/src/main/java/org/apache/pinot/core/common/datatable/DataTableBuilderFactory.java

Line 37 in 29fd5d9

public static final int DEFAULT_VERSION = DataTableFactory.VERSION_3;

Good point, any idea this doesn't fail on integration test?

cc: @walterddr

datatable version is set to 4 in multistage integration tests

see: https://github.com/apache/pinot/pull/10543/files#diff-9724fd2cc0075a3c9c9a29cc7b98bb921cfe2690c2cc0b6e0ad47dec0589e6abL83

walterddr

lgtm. we can also have a restart integration test that basically

start broker/server
tear it down and start them again (in server/broker or broker/server order)
see if they still can query

walterddr · 2023-04-06T03:59:12Z

test failure seems reproducible in 2 CI runs. can you take a look?

xiangfu0 · 2023-04-06T21:54:53Z

Modified the V4 DataTable _variableSizeDataBytes to byte[0] instead of null.
This is due to the only value of byte[0] is set, but read as null in deserialize ByteBuffer.

Before serialization

After Deserialization

xiangfu0 · 2023-04-07T02:09:44Z

pinot-common/src/main/java/org/apache/pinot/common/datatable/DataTableImplV4.java

@@ -150,15 +150,12 @@ public DataTableImplV4(ByteBuffer byteBuffer)
    }

    // Read variable size data.
+    _variableSizeDataBytes = new byte[variableSizeDataLength];


@walterddr Please verify this logic.

this should be ok.

xiangfu0 requested review from walterddr and Jackie-Jiang April 4, 2023 21:37

xiangfu0 force-pushed the enable-v2-engine-by-default branch from cb5f727 to a29d141 Compare April 4, 2023 21:41

xiangfu0 requested a review from siddharthteotia April 4, 2023 21:55

walterddr reviewed Apr 4, 2023

View reviewed changes

xiangfu0 force-pushed the enable-v2-engine-by-default branch 3 times, most recently from 145c20b to 107db8a Compare April 5, 2023 09:45

siddharthteotia reviewed Apr 5, 2023

View reviewed changes

ankitsultana reviewed Apr 5, 2023

View reviewed changes

xiangfu0 force-pushed the enable-v2-engine-by-default branch 3 times, most recently from 6081fa6 to 60a7927 Compare April 5, 2023 21:27

walterddr approved these changes Apr 5, 2023

View reviewed changes

xiangfu0 added 2 commits April 6, 2023 12:10

Turn on v2 engine by default

cf8ccfe

Update broker instance config in helix

c3d6076

xiangfu0 force-pushed the enable-v2-engine-by-default branch from 60a7927 to a7bf88a Compare April 6, 2023 19:10

Update default DataTable version to 4

a8b222c

xiangfu0 force-pushed the enable-v2-engine-by-default branch from a7bf88a to a8b222c Compare April 6, 2023 21:51

xiangfu0 commented Apr 7, 2023

View reviewed changes

xiangfu0 merged commit 8eaeb03 into apache:master Apr 7, 2023

Jackie-Jiang mentioned this pull request Jul 21, 2023

Deprecate older data tables #11140

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Turn on v2 engine by default #10543

Turn on v2 engine by default #10543

xiangfu0 commented Apr 4, 2023 •

edited

Loading

siddharthteotia commented Apr 4, 2023

xiangfu0 commented Apr 4, 2023

walterddr Apr 4, 2023

xiangfu0 Apr 4, 2023

xiangfu0 Apr 5, 2023

codecov-commenter commented Apr 4, 2023 •

edited

Loading

siddharthteotia Apr 5, 2023

walterddr Apr 5, 2023

xiangfu0 Apr 5, 2023

ankitsultana Apr 5, 2023 •

edited

Loading

xiangfu0 Apr 5, 2023

xiangfu0 Apr 5, 2023

walterddr Apr 5, 2023

walterddr Apr 5, 2023

walterddr left a comment

walterddr commented Apr 6, 2023

xiangfu0 commented Apr 6, 2023 •

edited

Loading

xiangfu0 Apr 7, 2023

walterddr Apr 7, 2023

Turn on v2 engine by default #10543

Turn on v2 engine by default #10543

Conversation

xiangfu0 commented Apr 4, 2023 • edited Loading

siddharthteotia commented Apr 4, 2023

xiangfu0 commented Apr 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Apr 4, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ankitsultana Apr 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

walterddr left a comment

Choose a reason for hiding this comment

walterddr commented Apr 6, 2023

xiangfu0 commented Apr 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xiangfu0 commented Apr 4, 2023 •

edited

Loading

codecov-commenter commented Apr 4, 2023 •

edited

Loading

ankitsultana Apr 5, 2023 •

edited

Loading

xiangfu0 commented Apr 6, 2023 •

edited

Loading