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/06/02 23:25:12 UTC

[GitHub] [pinot] mqliang opened a new pull request, #8820: Not overwrite the whole SystemResourceInfo config map

mqliang opened a new pull request, #8820:
URL: https://github.com/apache/pinot/pull/8820

   https://github.com/apache/pinot/pull/8785 put InstanceResourceInfo updating logic into a private method `private void startupServiceStatusCheck(long endTimeMs)` method, which make overriding the InstanceResourceInfo logic impossible.
   
   This PR extracts the logic back into a protect method.
   
   @jackjlli 


-- 
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] mqliang commented on a diff in pull request #8820: Extract InstanceResourceInfo updating logic into a protected method to allow overriding

Posted by GitBox <gi...@apache.org>.
mqliang commented on code in PR #8820:
URL: https://github.com/apache/pinot/pull/8820#discussion_r888407388


##########
pinot-server/src/main/java/org/apache/pinot/server/starter/helix/BaseServerStarter.java:
##########
@@ -346,7 +346,7 @@ private void updateInstanceConfigIfNeeded(ServerConf serverConf) {
     Map<String, String> systemResourceInfoMap = new SystemResourceInfo().toMap();
     if (!systemResourceInfoMap.equals(znRecord.getMapField(Instance.SYSTEM_RESOURCE_INFO_KEY))) {

Review Comment:
   > It'd be good to check whether the new kv pairs from the new map equal to the ones appeared from ZNRecord.
   
   we already do the check:
   ```
   if (!systemResourceInfoMap.equals(znRecord.getMapField(Instance.SYSTEM_RESOURCE_INFO_KEY)))
   ```
   



-- 
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 merged pull request #8820: Not overwrite the whole SystemResourceInfo config map

Posted by GitBox <gi...@apache.org>.
jackjlli merged PR #8820:
URL: https://github.com/apache/pinot/pull/8820


-- 
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 diff in pull request #8820: Not overwrite the whole SystemResourceInfo config map

Posted by GitBox <gi...@apache.org>.
jackjlli commented on code in PR #8820:
URL: https://github.com/apache/pinot/pull/8820#discussion_r888482844


##########
pinot-server/src/main/java/org/apache/pinot/server/starter/helix/BaseServerStarter.java:
##########
@@ -343,10 +343,21 @@ private void updateInstanceConfigIfNeeded(ServerConf serverConf) {
     }
 
     // Update system resource info (CPU, memory, etc)
-    Map<String, String> systemResourceInfoMap = new SystemResourceInfo().toMap();
-    if (!systemResourceInfoMap.equals(znRecord.getMapField(Instance.SYSTEM_RESOURCE_INFO_KEY))) {
-      LOGGER.info("Updating instance: {} with system resource info: {}", _instanceId, systemResourceInfoMap);
-      znRecord.setMapField(Instance.SYSTEM_RESOURCE_INFO_KEY, systemResourceInfoMap);
+    Map<String, String> newSystemResourceInfoMap = new SystemResourceInfo().toMap();
+    Map<String, String> oldSystemResourceInfoMap =

Review Comment:
   sed `s/oldSystemResourceInfoMap/existingSystemResourceInfoMap/`



-- 
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 diff in pull request #8820: Extract InstanceResourceInfo updating logic into a protected method to allow overriding

Posted by GitBox <gi...@apache.org>.
jackjlli commented on code in PR #8820:
URL: https://github.com/apache/pinot/pull/8820#discussion_r888303458


##########
pinot-server/src/main/java/org/apache/pinot/server/starter/helix/BaseServerStarter.java:
##########
@@ -751,6 +751,21 @@ public ServerInstance getServerInstance() {
     return _serverInstance;
   }
 
+  /**
+   * Helper method to set system resource info into instance config.
+   *
+   * @param helixAdmin Helix Admin
+   * @param helixClusterName Name of Helix cluster
+   * @param instanceId Id of instance for which to set the system resource info
+   * @param systemResourceMap Map containing system resource info
+   */
+  protected void setInstanceResourceInfo(HelixAdmin helixAdmin, String helixClusterName, String instanceId,
+      Map<String, String> systemResourceMap) {
+    InstanceConfig instanceConfig = helixAdmin.getInstanceConfig(helixClusterName, instanceId);
+    instanceConfig.getRecord().setMapField(Helix.Instance.SYSTEM_RESOURCE_INFO_KEY, systemResourceMap);
+    helixAdmin.setInstanceConfig(helixClusterName, instanceId, instanceConfig);

Review Comment:
   You don't need to call this setter method one more time as it'll be done at the end. Please refer to the other logic in the same class.



-- 
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] mqliang commented on a diff in pull request #8820: Extract InstanceResourceInfo updating logic into a protected method to allow overriding

Posted by GitBox <gi...@apache.org>.
mqliang commented on code in PR #8820:
URL: https://github.com/apache/pinot/pull/8820#discussion_r888319066


##########
pinot-server/src/main/java/org/apache/pinot/server/starter/helix/BaseServerStarter.java:
##########
@@ -751,6 +751,21 @@ public ServerInstance getServerInstance() {
     return _serverInstance;
   }
 
+  /**
+   * Helper method to set system resource info into instance config.
+   *
+   * @param helixAdmin Helix Admin
+   * @param helixClusterName Name of Helix cluster
+   * @param instanceId Id of instance for which to set the system resource info
+   * @param systemResourceMap Map containing system resource info
+   */
+  protected void setInstanceResourceInfo(HelixAdmin helixAdmin, String helixClusterName, String instanceId,
+      Map<String, String> systemResourceMap) {
+    InstanceConfig instanceConfig = helixAdmin.getInstanceConfig(helixClusterName, instanceId);
+    instanceConfig.getRecord().setMapField(Helix.Instance.SYSTEM_RESOURCE_INFO_KEY, systemResourceMap);
+    helixAdmin.setInstanceConfig(helixClusterName, instanceId, instanceConfig);

Review Comment:
   done



-- 
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 diff in pull request #8820: Extract InstanceResourceInfo updating logic into a protected method to allow overriding

Posted by GitBox <gi...@apache.org>.
jackjlli commented on code in PR #8820:
URL: https://github.com/apache/pinot/pull/8820#discussion_r888402755


##########
pinot-server/src/main/java/org/apache/pinot/server/starter/helix/BaseServerStarter.java:
##########
@@ -751,6 +751,20 @@ public ServerInstance getServerInstance() {
     return _serverInstance;
   }
 
+  /**
+   * Helper method to set system resource info into instance config.
+   *
+   * @param helixAdmin Helix Admin
+   * @param helixClusterName Name of Helix cluster
+   * @param instanceId Id of instance for which to set the system resource info
+   * @param systemResourceMap Map containing system resource info
+   */
+  protected void setInstanceResourceInfo(HelixAdmin helixAdmin, String helixClusterName, String instanceId,
+      Map<String, String> systemResourceMap) {
+    InstanceConfig instanceConfig = helixAdmin.getInstanceConfig(helixClusterName, instanceId);

Review Comment:
   You don't need to fetch instance config again. Please refer to the other logic in the same method.



##########
pinot-server/src/main/java/org/apache/pinot/server/starter/helix/BaseServerStarter.java:
##########
@@ -346,7 +346,7 @@ private void updateInstanceConfigIfNeeded(ServerConf serverConf) {
     Map<String, String> systemResourceInfoMap = new SystemResourceInfo().toMap();
     if (!systemResourceInfoMap.equals(znRecord.getMapField(Instance.SYSTEM_RESOURCE_INFO_KEY))) {

Review Comment:
   The existing map from ZNRecord can contain more information than the default map generated from `SystemResourceInfo` class. It'd be good to check whether the new kv pairs from the new map equal to the ones appeared from ZNRecord.



-- 
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 #8820: Not overwrite the whole SystemResourceInfo config map

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

   # [Codecov](https://codecov.io/gh/apache/pinot/pull/8820?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 [#8820](https://codecov.io/gh/apache/pinot/pull/8820?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (93ff103) into [master](https://codecov.io/gh/apache/pinot/commit/8788f1c0c31006b4610993397a6272a25097d0da?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (8788f1c) will **decrease** coverage by `42.82%`.
   > The diff coverage is `4.95%`.
   
   > :exclamation: Current head 93ff103 differs from pull request most recent head 6f7ac3e. Consider uploading reports for the commit 6f7ac3e to get more accurate results
   
   ```diff
   @@              Coverage Diff              @@
   ##             master    #8820       +/-   ##
   =============================================
   - Coverage     69.75%   26.93%   -42.83%     
   + Complexity     4624        1     -4623     
   =============================================
     Files          1735     1729        -6     
     Lines         91308    91131      -177     
     Branches      13638    13638               
   =============================================
   - Hits          63690    24544    -39146     
   - Misses        23201    64249    +41048     
   + Partials       4417     2338     -2079     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | integration1 | `26.93% <4.95%> (-0.03%)` | :arrow_down: |
   | integration2 | `?` | |
   | unittests1 | `?` | |
   | 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/8820?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [...ent/local/dedup/PartitionDedupMetadataManager.java](https://codecov.io/gh/apache/pinot/pull/8820/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-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9kZWR1cC9QYXJ0aXRpb25EZWR1cE1ldGFkYXRhTWFuYWdlci5qYXZh) | `0.00% <0.00%> (ø)` | |
   | [...segment/local/dedup/TableDedupMetadataManager.java](https://codecov.io/gh/apache/pinot/pull/8820/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-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9kZWR1cC9UYWJsZURlZHVwTWV0YWRhdGFNYW5hZ2VyLmphdmE=) | `0.00% <0.00%> (ø)` | |
   | [...l/indexsegment/immutable/ImmutableSegmentImpl.java](https://codecov.io/gh/apache/pinot/pull/8820/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-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9pbmRleHNlZ21lbnQvaW1tdXRhYmxlL0ltbXV0YWJsZVNlZ21lbnRJbXBsLmphdmE=) | `0.00% <0.00%> (-68.12%)` | :arrow_down: |
   | [...local/indexsegment/mutable/MutableSegmentImpl.java](https://codecov.io/gh/apache/pinot/pull/8820/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-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9pbmRleHNlZ21lbnQvbXV0YWJsZS9NdXRhYmxlU2VnbWVudEltcGwuamF2YQ==) | `0.00% <0.00%> (-59.29%)` | :arrow_down: |
   | [...ent/local/realtime/impl/RealtimeSegmentConfig.java](https://codecov.io/gh/apache/pinot/pull/8820/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-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC9yZWFsdGltZS9pbXBsL1JlYWx0aW1lU2VnbWVudENvbmZpZy5qYXZh) | `0.00% <0.00%> (-92.37%)` | :arrow_down: |
   | [...not/segment/local/upsert/PartialUpsertHandler.java](https://codecov.io/gh/apache/pinot/pull/8820/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-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC91cHNlcnQvUGFydGlhbFVwc2VydEhhbmRsZXIuamF2YQ==) | `0.00% <0.00%> (-38.71%)` | :arrow_down: |
   | [...t/local/upsert/PartitionUpsertMetadataManager.java](https://codecov.io/gh/apache/pinot/pull/8820/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-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC91cHNlcnQvUGFydGl0aW9uVXBzZXJ0TWV0YWRhdGFNYW5hZ2VyLmphdmE=) | `0.00% <0.00%> (-76.00%)` | :arrow_down: |
   | [...gment/local/upsert/TableUpsertMetadataManager.java](https://codecov.io/gh/apache/pinot/pull/8820/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-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC91cHNlcnQvVGFibGVVcHNlcnRNZXRhZGF0YU1hbmFnZXIuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [...rg/apache/pinot/segment/local/utils/HashUtils.java](https://codecov.io/gh/apache/pinot/pull/8820/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-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC91dGlscy9IYXNoVXRpbHMuamF2YQ==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [...g/apache/pinot/segment/local/utils/RecordInfo.java](https://codecov.io/gh/apache/pinot/pull/8820/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-cGlub3Qtc2VnbWVudC1sb2NhbC9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3Qvc2VnbWVudC9sb2NhbC91dGlscy9SZWNvcmRJbmZvLmphdmE=) | `0.00% <0.00%> (ø)` | |
   | ... and [1260 more](https://codecov.io/gh/apache/pinot/pull/8820/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/8820?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/8820?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 [8788f1c...6f7ac3e](https://codecov.io/gh/apache/pinot/pull/8820?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] mqliang closed pull request #8820: Not overwrite the whole SystemResourceInfo config map

Posted by GitBox <gi...@apache.org>.
mqliang closed pull request #8820: Not overwrite the whole SystemResourceInfo config map
URL: https://github.com/apache/pinot/pull/8820


-- 
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 diff in pull request #8820: Not overwrite the whole SystemResourceInfo config map

Posted by GitBox <gi...@apache.org>.
jackjlli commented on code in PR #8820:
URL: https://github.com/apache/pinot/pull/8820#discussion_r888460056


##########
pinot-server/src/main/java/org/apache/pinot/server/starter/helix/BaseServerStarter.java:
##########
@@ -343,10 +343,17 @@ private void updateInstanceConfigIfNeeded(ServerConf serverConf) {
     }
 
     // Update system resource info (CPU, memory, etc)
-    Map<String, String> systemResourceInfoMap = new SystemResourceInfo().toMap();
-    if (!systemResourceInfoMap.equals(znRecord.getMapField(Instance.SYSTEM_RESOURCE_INFO_KEY))) {
-      LOGGER.info("Updating instance: {} with system resource info: {}", _instanceId, systemResourceInfoMap);
-      znRecord.setMapField(Instance.SYSTEM_RESOURCE_INFO_KEY, systemResourceInfoMap);
+    Map<String, String> newSystemResourceInfoMap = new SystemResourceInfo().toMap();
+    Map<String, String> oldSystemResourceInfoMap =
+        znRecord.getMapField(CommonConstants.Helix.Instance.SYSTEM_RESOURCE_INFO_KEY);
+    if (!newSystemResourceInfoMap.equals(oldSystemResourceInfoMap)) {
+      LOGGER.info("Updating instance: {} with new system resource info: {}", _instanceId, newSystemResourceInfoMap);
+      // oldSystemResourceInfoMap may contains more KV pairs than newSystemResourceInfoMap, we need add the extra KV
+      // pairs into newSystemResourceInfoMap.
+      for (Map.Entry<String, String> entry : oldSystemResourceInfoMap.entrySet()) {

Review Comment:
   This logic seems incorrect. You now basically replace the new KV pair with old value.



-- 
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