Fix schema name in table config during controller startup #11574

xiangfu0 · 2023-09-12T09:26:58Z

Fix schema name in table config during controller startup:

If schema name is already null in table config -> GOOD
If rawTable name is already in the schema, then likely there is a misconfiguration, set the schema name to null and update table config.
Copy referred schema to the same name of rawTable, and set schema name to null in table config.

codecov-commenter · 2023-09-12T10:21:52Z

Codecov Report

Merging #11574 (5e965bd) into master (17ffa3e) will increase coverage by 0.01%.
Report is 1 commits behind head on master.
The diff coverage is 59.45%.

@@             Coverage Diff              @@
##             master   #11574      +/-   ##
============================================
+ Coverage     63.09%   63.11%   +0.01%     
  Complexity     1117     1117              
============================================
  Files          2342     2342              
  Lines        125920   125993      +73     
  Branches      19370    19377       +7     
============================================
+ Hits          79454    79517      +63     
- Misses        40812    40818       +6     
- Partials       5654     5658       +4

Flag	Coverage Δ
integration	`<0.01% <0.00%> (ø)`
integration1	`<0.01% <0.00%> (ø)`
integration2	`0.00% <0.00%> (ø)`
java-11	`63.06% <59.45%> (+0.01%)`	⬆️
java-17	`62.94% <59.45%> (-0.02%)`	⬇️
java-20	`62.96% <59.45%> (+<0.01%)`	⬆️
temurin	`63.11% <59.45%> (+0.01%)`	⬆️
unittests	`63.10% <59.45%> (+0.01%)`	⬆️
unittests1	`67.25% <0.00%> (+0.01%)`	⬆️
unittests2	`14.44% <59.45%> (+0.02%)`	⬆️

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

Files	Coverage Δ
...g/apache/pinot/common/metrics/ControllerGauge.java	`0.00% <0.00%> (ø)`
...ache/pinot/common/metadata/ZKMetadataProvider.java	`11.00% <0.00%> (-0.50%)`	⬇️
...apache/pinot/controller/BaseControllerStarter.java	`77.64% <74.57%> (-0.52%)`	⬇️

... and 14 files with indirect coverage changes

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

Jackie-Jiang · 2023-09-16T20:54:11Z

We don't really need a periodic task after preventing new table with different schema to be created. It can be done as a one time scan when controller starts

Jackie-Jiang · 2023-09-16T20:55:09Z

Actually a periodic task is also fine. We will remove it after one release anyway

xiangfu0 · 2023-09-19T04:40:13Z

Actually a periodic task is also fine. We will remove it after one release anyway

I would say we can keep this job for 1.x release, then remove it in 2.x :)

eaugene · 2023-09-19T05:54:40Z

@xiangfu0 , I am not able to understand - Why are we needing a periodic task for this , as new tables creation with different schema name is anyways going to blocked . Any problems we encounter by keeping this -

A one time task ( As Jackie pointed out , we can limit it during controller spawn up ) ?
To have it more transparent to table owners , giving the control to them to trigger this action by a REST API ?

xiangfu0 · 2023-09-19T08:20:23Z

@xiangfu0 , I am not able to understand - Why are we needing a periodic task for this , as new tables creation with different schema name is anyways going to blocked . Any problems we encounter by keeping this -

A one time task ( As Jackie pointed out , we can limit it during controller spawn up ) ?

To have it more transparent to table owners , giving the control to them to trigger this action by a REST API ?

My feeling is #11570 may not get merged before this PR :p

The purpose is to ensure each table has its own schema, no cross references.

pinot-controller/src/main/java/org/apache/pinot/controller/BaseControllerStarter.java

jackjlli

LGTM. Thanks for adding the code changes!

xiangfu0 · 2023-10-04T21:27:00Z

Rebased to incorporate the changes from #11591

Jackie-Jiang

We can make the counter more specific:

Tables without schema
Misconfigured tables: tables with schema name not matching the raw table name
Fixed tables
Failed to copy schema
Failed to update table config

pinot-controller/src/main/java/org/apache/pinot/controller/ControllerConf.java

pinot-controller/src/main/java/org/apache/pinot/controller/BaseControllerStarter.java

Jackie-Jiang · 2023-10-06T21:07:08Z

pinot-common/src/main/java/org/apache/pinot/common/metrics/ControllerGauge.java

+  MAX_RECORD_AVAILABILITY_LAG_MS("maxRecordAvailabilityLagMs", false),
+
+  // Number of table schema got fixed
+  FIXED_SCHEMA_TABLE_COUNT("FixedSchemaTableCount", true),


Do we have these configs covered in the prometheus yaml?

those should be covered by the match all rule

pinot-controller/src/main/java/org/apache/pinot/controller/BaseControllerStarter.java

Jackie-Jiang · 2023-10-06T21:17:15Z

pinot-controller/src/main/java/org/apache/pinot/controller/BaseControllerStarter.java

+      // Update table config to remove schema name
+      tableConfig.getValidationConfig().setSchemaName(null);
+      try {
+        _helixResourceManager.updateTableConfig(tableConfig);


We need to do version check and update. Currently it might override another table config update.

I would suggest to do this in another PR for helixResourceManager API?

We don't need to change PinotHelixResouceManager. Actually we should not even use this updateTableConfig() method, but should use property store instead which can check the versions.
PinotHelixResourceManager.updateTableConfig() is always overriding the whole table config, instead of changing a field, and that is why version check is not required

Jackie-Jiang · 2023-10-10T23:55:23Z

pinot-common/src/main/java/org/apache/pinot/common/metadata/ZKMetadataProvider.java

+      return propertyStore.set(constructPropertyStorePathForResourceConfig(tableNameWithType), tableConfigZNRecord,
+          expectVersion, AccessOption.PERSISTENT);


This part should be outside of the try-catch

Jackie-Jiang · 2023-10-10T23:56:02Z

pinot-common/src/main/java/org/apache/pinot/common/metadata/ZKMetadataProvider.java

+   *
+   * @return true if update is successful.
+   */
+  public static boolean setTableConfig(ZkHelixPropertyStore<ZNRecord> propertyStore, String tableNameWithType,


(minor) tableNameWithType is not needed

Jackie-Jiang · 2023-10-10T23:57:59Z

pinot-common/src/main/java/org/apache/pinot/common/metadata/ZKMetadataProvider.java

+    TableConfig tableConfig = toTableConfig(
+        propertyStore.get(constructPropertyStorePathForResourceConfig(tableNameWithType), tableConfigStat,
+            AccessOption.PERSISTENT));
+    return ImmutablePair.of(tableConfig, tableConfigStat.getVersion());


When tableConfig is null, we should probably directly return null instead of a pair. Also annotate the return as @Nullable

xiangfu0 force-pushed the update_schema_with_table_name branch from ee7b25b to eb503fd Compare September 12, 2023 09:31

xiangfu0 force-pushed the update_schema_with_table_name branch 5 times, most recently from 14f5349 to 19e9346 Compare September 13, 2023 15:17

xiangfu0 requested review from Jackie-Jiang and npawar September 13, 2023 15:26

xiangfu0 force-pushed the update_schema_with_table_name branch from 19e9346 to 0c9aeaa Compare September 13, 2023 15:29

xiangfu0 mentioned this pull request Sep 15, 2023

Throw exception when schema name doesn't match table name during table creation #11591

Merged

xiangfu0 force-pushed the update_schema_with_table_name branch from 0c9aeaa to d83ab8f Compare September 15, 2023 21:26

Jackie-Jiang mentioned this pull request Sep 19, 2023

Deprecate schema name field in table config #11570

Open

xiangfu0 force-pushed the update_schema_with_table_name branch from d83ab8f to 13dbc83 Compare September 19, 2023 21:38

xiangfu0 changed the title ~~Adding a controller task to update schema name~~ Fix schema name in table config during controller startup Sep 19, 2023

xiangfu0 force-pushed the update_schema_with_table_name branch 3 times, most recently from 6d81401 to 2019605 Compare September 19, 2023 23:46

xiangfu0 requested review from mcvsubbu, jackjlli, siddharthteotia and yupeng9 September 28, 2023 21:14

xiangfu0 added metrics cleanup incompatible Indicate PR that introduces backward incompatibility labels Sep 28, 2023

jackjlli reviewed Sep 28, 2023

View reviewed changes

pinot-controller/src/main/java/org/apache/pinot/controller/BaseControllerStarter.java Show resolved Hide resolved

xiangfu0 force-pushed the update_schema_with_table_name branch 2 times, most recently from 843e8bf to 409974a Compare September 29, 2023 06:05

xiangfu0 requested a review from jackjlli September 29, 2023 08:12

jackjlli approved these changes Oct 2, 2023

View reviewed changes

xiangfu0 force-pushed the update_schema_with_table_name branch 5 times, most recently from 004786d to fe68a6b Compare October 4, 2023 18:57

Jackie-Jiang reviewed Oct 6, 2023

View reviewed changes

xiangfu0 force-pushed the update_schema_with_table_name branch from fe68a6b to d0ea80e Compare October 9, 2023 20:58

xiangfu0 and others added 3 commits October 10, 2023 16:20

Adding a controller task to update schema name

45e62ec

Misc fixes

00493e2

Fix linter

313aa28

xiangfu0 force-pushed the update_schema_with_table_name branch 3 times, most recently from 0eb66f0 to 5e965bd Compare October 10, 2023 23:25

Jackie-Jiang reviewed Oct 10, 2023

View reviewed changes

xiangfu0 force-pushed the update_schema_with_table_name branch from 5e965bd to 313aa28 Compare October 11, 2023 00:43

xiangfu0 merged commit 169aba9 into apache:master Oct 11, 2023
17 of 19 checks passed

xiangfu0 deleted the update_schema_with_table_name branch October 11, 2023 00:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix schema name in table config during controller startup #11574

Fix schema name in table config during controller startup #11574

xiangfu0 commented Sep 12, 2023 •

edited

Loading

codecov-commenter commented Sep 12, 2023 •

edited

Loading

Jackie-Jiang commented Sep 16, 2023

Jackie-Jiang commented Sep 16, 2023

xiangfu0 commented Sep 19, 2023

eaugene commented Sep 19, 2023

xiangfu0 commented Sep 19, 2023 •

edited

Loading

jackjlli left a comment

xiangfu0 commented Oct 4, 2023 •

edited

Loading

Jackie-Jiang left a comment

Jackie-Jiang Oct 6, 2023

xiangfu0 Oct 9, 2023

Jackie-Jiang Oct 6, 2023

xiangfu0 Oct 9, 2023

Jackie-Jiang Oct 10, 2023

Jackie-Jiang Oct 10, 2023

Jackie-Jiang Oct 10, 2023

Jackie-Jiang Oct 10, 2023

		return propertyStore.set(constructPropertyStorePathForResourceConfig(tableNameWithType), tableConfigZNRecord,
		expectVersion, AccessOption.PERSISTENT);

Fix schema name in table config during controller startup #11574

Fix schema name in table config during controller startup #11574

Conversation

xiangfu0 commented Sep 12, 2023 • edited Loading

codecov-commenter commented Sep 12, 2023 • edited Loading

Codecov Report

Jackie-Jiang commented Sep 16, 2023

Jackie-Jiang commented Sep 16, 2023

xiangfu0 commented Sep 19, 2023

eaugene commented Sep 19, 2023

xiangfu0 commented Sep 19, 2023 • edited Loading

jackjlli left a comment

Choose a reason for hiding this comment

xiangfu0 commented Oct 4, 2023 • edited Loading

Jackie-Jiang left a 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

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

xiangfu0 commented Sep 12, 2023 •

edited

Loading

codecov-commenter commented Sep 12, 2023 •

edited

Loading

xiangfu0 commented Sep 19, 2023 •

edited

Loading

xiangfu0 commented Oct 4, 2023 •

edited

Loading