Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix AddTable for realtime tables #9506

Merged
merged 2 commits into from
Sep 30, 2022
Merged

Fix AddTable for realtime tables #9506

merged 2 commits into from
Sep 30, 2022

Conversation

npawar
Copy link
Contributor

@npawar npawar commented Sep 30, 2022

Fixing a bug happened as a side effect of #9228

Due to the bug, we lost the ability to add realtime tables using AddTable. The logic was changed to use the tableConfigs API, and it assumed that config will always be offline. An oss user stumbled upon this after upgrading to 0.11 https://apache-pinot.slack.com/archives/C011C9JHN7R/p1664525974774459

After the fix, we will be able to provide both table configs to the command using -realtimeTableConfigFile and -offlineTableConfigFile, and if you only provide -tableConfigFile, the right tableType will be extracted from the table config.

@npawar npawar added the bugfix label Sep 30, 2022
@npawar npawar requested a review from walterddr September 30, 2022 18:06
Copy link
Contributor

@walterddr walterddr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good. have one quick question

Comment on lines 217 to 222
TableType tableType = TableNameBuilder.getTableTypeFromTableName(tableConfig.getTableName());
if (tableType == TableType.OFFLINE) {
offlineTableConfig = tableConfig;
} else {
realtimeTableConfig = tableConfig;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so the table is default to REALTIME if no type is extracted out?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we enforce this and throw if table type is unknown?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually realized that we can just get tableType directly from config, no extraction needed. updated

@codecov-commenter
Copy link

codecov-commenter commented Sep 30, 2022

Codecov Report

Merging #9506 (9dd8a01) into master (03b9d4a) will increase coverage by 0.00%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             master    #9506    +/-   ##
==========================================
  Coverage     69.98%   69.99%            
- Complexity     4869     5199   +330     
==========================================
  Files          1921     1921            
  Lines        102356   102356            
  Branches      15532    15532            
==========================================
+ Hits          71636    71646    +10     
+ Misses        25658    25648    -10     
  Partials       5062     5062            
Flag Coverage Δ
integration1 26.06% <ø> (+0.19%) ⬆️
integration2 24.69% <ø> (-0.01%) ⬇️
unittests1 67.29% <ø> (-0.02%) ⬇️
unittests2 15.61% <ø> (+0.02%) ⬆️

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

Impacted Files Coverage Δ
...ore/query/scheduler/resources/ResourceManager.java 84.00% <0.00%> (-12.00%) ⬇️
...inot/core/util/SegmentCompletionProtocolUtils.java 57.69% <0.00%> (-7.70%) ⬇️
...mmon/request/context/predicate/NotEqPredicate.java 76.92% <0.00%> (-7.70%) ⬇️
...tream/kafka20/server/KafkaDataServerStartable.java 72.91% <0.00%> (-6.25%) ⬇️
.../pinot/core/query/scheduler/PriorityScheduler.java 77.77% <0.00%> (-5.56%) ⬇️
...roller/helix/core/relocation/SegmentRelocator.java 72.97% <0.00%> (-5.41%) ⬇️
...ot/segment/local/startree/OffHeapStarTreeNode.java 65.90% <0.00%> (-4.55%) ⬇️
...perator/filter/SortedIndexBasedFilterOperator.java 83.80% <0.00%> (-3.81%) ⬇️
...nction/DistinctCountBitmapAggregationFunction.java 47.66% <0.00%> (-2.08%) ⬇️
...ata/manager/realtime/RealtimeTableDataManager.java 68.19% <0.00%> (-0.77%) ⬇️
... and 15 more

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

@npawar npawar merged commit a12c54f into apache:master Sep 30, 2022
@npawar npawar deleted the add_table_fix branch September 30, 2022 20:31
@mcvsubbu
Copy link
Contributor

@npawar can you just add a note on what the bug was? We pull from the master, so we may have pulled the buggy version. It will be useful to know so we can warn our users (or expect some strange behavior). thanks

@npawar
Copy link
Contributor Author

npawar commented Sep 30, 2022

@npawar can you just add a note on what the bug was? We pull from the master, so we may have pulled the buggy version. It will be useful to know so we can warn our users (or expect some strange behavior). thanks

done

61yao pushed a commit to 61yao/pinot that referenced this pull request Oct 3, 2022
* Fix AddTable for realtime tables

* Type
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants