You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by GitBox <gi...@apache.org> on 2022/01/12 17:31:21 UTC

[GitHub] [pinot] npawar opened a new pull request #8004: Rename TableCache to DefaultPinotConfigProvider

npawar opened a new pull request #8004:
URL: https://github.com/apache/pinot/pull/8004


   Followup to adding PinotConfigProvider interface, renaming the impl from TableCache to DefaultPinotConfigProvider.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [pinot] codecov-commenter commented on pull request #8004: Rename TableCache to DefaultPinotConfigProvider

Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #8004:
URL: https://github.com/apache/pinot/pull/8004#issuecomment-1011337204


   # [Codecov](https://codecov.io/gh/apache/pinot/pull/8004?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#8004](https://codecov.io/gh/apache/pinot/pull/8004?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (30d90b2) into [master](https://codecov.io/gh/apache/pinot/commit/2d81c4741ebd18d389f54f3acb3ab03aaf8c7c00?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (2d81c47) will **decrease** coverage by `0.75%`.
   > The diff coverage is `73.17%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/pinot/pull/8004/graphs/tree.svg?width=650&height=150&src=pr&token=4ibza2ugkz&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)](https://codecov.io/gh/apache/pinot/pull/8004?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   
   ```diff
   @@             Coverage Diff              @@
   ##             master    #8004      +/-   ##
   ============================================
   - Coverage     64.86%   64.10%   -0.76%     
   + Complexity     4218     4134      -84     
   ============================================
     Files          1551     1587      +36     
     Lines         80889    82435    +1546     
     Branches      12146    12309     +163     
   ============================================
   + Hits          52467    52846     +379     
   - Misses        24695    25817    +1122     
   - Partials       3727     3772      +45     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | integration1 | `29.00% <68.29%> (?)` | |
   | integration2 | `27.56% <73.17%> (?)` | |
   | unittests1 | `68.12% <2.50%> (-0.07%)` | :arrow_down: |
   | unittests2 | `?` | |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://codecov.io/gh/apache/pinot/pull/8004?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [.../core/realtime/PinotLLCRealtimeSegmentManager.java](https://codecov.io/gh/apache/pinot/pull/8004/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL3JlYWx0aW1lL1Bpbm90TExDUmVhbHRpbWVTZWdtZW50TWFuYWdlci5qYXZh) | `62.37% <ø> (-5.09%)` | :arrow_down: |
   | [...manager/realtime/LLRealtimeSegmentDataManager.java](https://codecov.io/gh/apache/pinot/pull/8004/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS9kYXRhL21hbmFnZXIvcmVhbHRpbWUvTExSZWFsdGltZVNlZ21lbnREYXRhTWFuYWdlci5qYXZh) | `70.47% <50.00%> (+25.19%)` | :arrow_up: |
   | [...ntroller/helix/core/PinotHelixResourceManager.java](https://codecov.io/gh/apache/pinot/pull/8004/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9oZWxpeC9jb3JlL1Bpbm90SGVsaXhSZXNvdXJjZU1hbmFnZXIuamF2YQ==) | `43.62% <57.14%> (-17.10%)` | :arrow_down: |
   | [...on/config/provider/DefaultPinotConfigProvider.java](https://codecov.io/gh/apache/pinot/pull/8004/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vY29uZmlnL3Byb3ZpZGVyL0RlZmF1bHRQaW5vdENvbmZpZ1Byb3ZpZGVyLmphdmE=) | `77.60% <68.42%> (ø)` | |
   | [...roker/requesthandler/BaseBrokerRequestHandler.java](https://codecov.io/gh/apache/pinot/pull/8004/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcmVxdWVzdGhhbmRsZXIvQmFzZUJyb2tlclJlcXVlc3RIYW5kbGVyLmphdmE=) | `64.14% <79.31%> (+42.63%)` | :arrow_up: |
   | [...e/pinot/broker/broker/helix/BaseBrokerStarter.java](https://codecov.io/gh/apache/pinot/pull/8004/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvYnJva2VyL2hlbGl4L0Jhc2VCcm9rZXJTdGFydGVyLmphdmE=) | `73.03% <100.00%> (+1.12%)` | :arrow_up: |
   | [...roker/requesthandler/GrpcBrokerRequestHandler.java](https://codecov.io/gh/apache/pinot/pull/8004/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcmVxdWVzdGhhbmRsZXIvR3JwY0Jyb2tlclJlcXVlc3RIYW5kbGVyLmphdmE=) | `78.04% <100.00%> (+78.04%)` | :arrow_up: |
   | [...thandler/SingleConnectionBrokerRequestHandler.java](https://codecov.io/gh/apache/pinot/pull/8004/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtYnJva2VyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9icm9rZXIvcmVxdWVzdGhhbmRsZXIvU2luZ2xlQ29ubmVjdGlvbkJyb2tlclJlcXVlc3RIYW5kbGVyLmphdmE=) | `87.03% <100.00%> (+74.07%)` | :arrow_up: |
   | [...pinot/controller/recommender/io/ConfigManager.java](https://codecov.io/gh/apache/pinot/pull/8004/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9yZWNvbW1lbmRlci9pby9Db25maWdNYW5hZ2VyLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [...troller/recommender/io/metadata/FieldMetadata.java](https://codecov.io/gh/apache/pinot/pull/8004/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGlub3QtY29udHJvbGxlci9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29udHJvbGxlci9yZWNvbW1lbmRlci9pby9tZXRhZGF0YS9GaWVsZE1ldGFkYXRhLmphdmE=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | ... and [531 more](https://codecov.io/gh/apache/pinot/pull/8004/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/pinot/pull/8004?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/pinot/pull/8004?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [7603a4f...30d90b2](https://codecov.io/gh/apache/pinot/pull/8004?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [pinot] npawar commented on a change in pull request #8004: Rename TableCache to DefaultPinotConfigProvider

Posted by GitBox <gi...@apache.org>.
npawar commented on a change in pull request #8004:
URL: https://github.com/apache/pinot/pull/8004#discussion_r783359353



##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/broker/helix/BaseBrokerStarter.java
##########
@@ -232,26 +232,27 @@ public void start()
     boolean caseInsensitive =
         _brokerConf.getProperty(Helix.ENABLE_CASE_INSENSITIVE_KEY, false) || _brokerConf.getProperty(
             Helix.DEPRECATED_ENABLE_CASE_INSENSITIVE_KEY, false);
-    TableCache tableCache = new TableCache(_propertyStore, caseInsensitive);
+    DefaultPinotConfigProvider defaultPinotConfigProvider =

Review comment:
       i tried to, but there's still `isCaseSensitive` and `getActualTableName` methods used from the impl. 
   I could just skip this PR and figure out what to do with those and then re-open, or I could just get the renaming in and then figure those out in a follow-up (figuring that might take some tie since I don't have the exact context on why thay are there in the first place)




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org

[GitHub] [pinot] jackjlli commented on a change in pull request #8004: Rename TableCache to DefaultPinotConfigProvider

Posted by GitBox <gi...@apache.org>.
jackjlli commented on a change in pull request #8004:
URL: https://github.com/apache/pinot/pull/8004#discussion_r783343221



##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/broker/helix/BaseBrokerStarter.java
##########
@@ -232,26 +232,27 @@ public void start()
     boolean caseInsensitive =
         _brokerConf.getProperty(Helix.ENABLE_CASE_INSENSITIVE_KEY, false) || _brokerConf.getProperty(
             Helix.DEPRECATED_ENABLE_CASE_INSENSITIVE_KEY, false);
-    TableCache tableCache = new TableCache(_propertyStore, caseInsensitive);
+    DefaultPinotConfigProvider defaultPinotConfigProvider =

Review comment:
       Probably use `PinotConfigProvider` as the reference here since we've already declared all the essential methods to the interface? Also change the variable name to `pinotConfigProvider`.

##########
File path: pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java
##########
@@ -111,7 +111,7 @@
   protected final RoutingManager _routingManager;
   protected final AccessControlFactory _accessControlFactory;
   protected final QueryQuotaManager _queryQuotaManager;
-  protected final TableCache _tableCache;
+  protected final DefaultPinotConfigProvider _defaultPinotConfigProvider;

Review comment:
       Use `PinotConfigProvider` instead?




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org