Re: [PR] Add instance level consumer dir usage metric [pinot]

2024-11-14 Thread via GitHub


jasperjiaguo merged PR #14430:
URL: https://github.com/apache/pinot/pull/14430


-- 
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-unsubscr...@pinot.apache.org

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


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



Re: [PR] Add instance level consumer dir usage metric [pinot]

2024-11-13 Thread via GitHub


SabrinaZhaozyf commented on code in PR #14430:
URL: https://github.com/apache/pinot/pull/14430#discussion_r1840938825


##
pinot-core/src/main/java/org/apache/pinot/core/data/manager/realtime/RealtimeTableDataManager.java:
##
@@ -102,7 +102,7 @@ public class RealtimeTableDataManager extends 
BaseTableDataManager {
   // The old name of the stats file used to be stats.ser which we changed when 
we moved all packages
   // from com.linkedin to org.apache because of not being able to deserialize 
the old files using the newer classes
   private static final String STATS_FILE_NAME = "segment-stats.ser";
-  private static final String CONSUMERS_DIR = "consumers";
+  public static final String CONSUMERS_DIR = "consumers";

Review Comment:
   done



##
pinot-common/src/main/java/org/apache/pinot/common/metrics/ServerGauge.java:
##
@@ -76,7 +76,8 @@ public enum ServerGauge implements AbstractMetrics.Gauge {
   // Needed to track if valid doc id snapshots are present for faster restarts
   UPSERT_VALID_DOC_ID_SNAPSHOT_COUNT("upsertValidDocIdSnapshotCount", false),
   UPSERT_PRIMARY_KEYS_IN_SNAPSHOT_COUNT("upsertPrimaryKeysInSnapshotCount", 
false),
-  REALTIME_INGESTION_OFFSET_LAG("offsetLag", false);
+  REALTIME_INGESTION_OFFSET_LAG("offsetLag", false),
+  CONSUMER_DIR_USAGE("bytes", true);

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-unsubscr...@pinot.apache.org

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


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



Re: [PR] Add instance level consumer dir usage metric [pinot]

2024-11-12 Thread via GitHub


vvivekiyer commented on code in PR #14430:
URL: https://github.com/apache/pinot/pull/14430#discussion_r1839194969


##
pinot-common/src/main/java/org/apache/pinot/common/metrics/ServerGauge.java:
##
@@ -76,7 +76,8 @@ public enum ServerGauge implements AbstractMetrics.Gauge {
   // Needed to track if valid doc id snapshots are present for faster restarts
   UPSERT_VALID_DOC_ID_SNAPSHOT_COUNT("upsertValidDocIdSnapshotCount", false),
   UPSERT_PRIMARY_KEYS_IN_SNAPSHOT_COUNT("upsertPrimaryKeysInSnapshotCount", 
false),
-  REALTIME_INGESTION_OFFSET_LAG("offsetLag", false);
+  REALTIME_INGESTION_OFFSET_LAG("offsetLag", false),
+  CONSUMER_DIR_USAGE("bytes", true);

Review Comment:
   (nit) can we name it as `REALTIME_CONSUMER_DIR_USAGE`? 



##
pinot-core/src/main/java/org/apache/pinot/core/data/manager/realtime/RealtimeTableDataManager.java:
##
@@ -102,7 +102,7 @@ public class RealtimeTableDataManager extends 
BaseTableDataManager {
   // The old name of the stats file used to be stats.ser which we changed when 
we moved all packages
   // from com.linkedin to org.apache because of not being able to deserialize 
the old files using the newer classes
   private static final String STATS_FILE_NAME = "segment-stats.ser";
-  private static final String CONSUMERS_DIR = "consumers";
+  public static final String CONSUMERS_DIR = "consumers";

Review Comment:
   Can we revert this to `private` now?



-- 
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-unsubscr...@pinot.apache.org

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


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



Re: [PR] Add instance level consumer dir usage metric [pinot]

2024-11-12 Thread via GitHub


SabrinaZhaozyf commented on code in PR #14430:
URL: https://github.com/apache/pinot/pull/14430#discussion_r1839096908


##
pinot-server/src/main/java/org/apache/pinot/server/starter/helix/HelixInstanceDataManager.java:
##
@@ -180,6 +181,24 @@ private void initInstanceDataDir(File instanceDataDir) {
 }
   }
 
+  @Override
+  public List getConsumerDirPaths() {
+String consumerDirPath = _instanceDataManagerConfig.getConsumerDir();
+
+if (consumerDirPath != null) {
+  return Collections.singletonList(new File(consumerDirPath));
+} else {
+  List consumerDirs = new ArrayList<>();
+  String instanceDataDir = _instanceDataManagerConfig.getInstanceDataDir();
+  for (String tableNameWithType : getAllTables()) {
+String dirPath = instanceDataDir + File.separator + tableNameWithType 
+ File.separator
++ RealtimeTableDataManager.CONSUMERS_DIR;
+consumerDirs.add(new File(dirPath));
+  }
+  return consumerDirs;

Review Comment:
   When consumerDir is not specified, `RealtimeTableDataManager` constructs a 
consumer directory under table data directory 
(`//consumers`). Else case here is to cover that 
case.



-- 
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-unsubscr...@pinot.apache.org

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


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



Re: [PR] Add instance level consumer dir usage metric [pinot]

2024-11-12 Thread via GitHub


SabrinaZhaozyf commented on code in PR #14430:
URL: https://github.com/apache/pinot/pull/14430#discussion_r1839102597


##
pinot-server/src/main/java/org/apache/pinot/server/starter/helix/HelixInstanceDataManager.java:
##
@@ -180,6 +181,24 @@ private void initInstanceDataDir(File instanceDataDir) {
 }
   }
 
+  @Override
+  public List getConsumerDirPaths() {
+String consumerDirPath = _instanceDataManagerConfig.getConsumerDir();
+
+if (consumerDirPath != null) {
+  return Collections.singletonList(new File(consumerDirPath));
+} else {
+  List consumerDirs = new ArrayList<>();
+  String instanceDataDir = _instanceDataManagerConfig.getInstanceDataDir();
+  for (String tableNameWithType : getAllTables()) {
+String dirPath = instanceDataDir + File.separator + tableNameWithType 
+ File.separator

Review Comment:
   It will not count offline table dataDir size as the directory will not 
exist. Latest commit explicitly checks instance type tho, which should make it 
clearer.



-- 
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-unsubscr...@pinot.apache.org

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


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



Re: [PR] Add instance level consumer dir usage metric [pinot]

2024-11-12 Thread via GitHub


SabrinaZhaozyf commented on code in PR #14430:
URL: https://github.com/apache/pinot/pull/14430#discussion_r1839101674


##
pinot-server/src/main/java/org/apache/pinot/server/starter/helix/HelixInstanceDataManager.java:
##
@@ -180,6 +181,24 @@ private void initInstanceDataDir(File instanceDataDir) {
 }
   }
 
+  @Override
+  public List getConsumerDirPaths() {
+String consumerDirPath = _instanceDataManagerConfig.getConsumerDir();
+
+if (consumerDirPath != null) {
+  return Collections.singletonList(new File(consumerDirPath));
+} else {
+  List consumerDirs = new ArrayList<>();

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-unsubscr...@pinot.apache.org

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


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



Re: [PR] Add instance level consumer dir usage metric [pinot]

2024-11-12 Thread via GitHub


SabrinaZhaozyf commented on code in PR #14430:
URL: https://github.com/apache/pinot/pull/14430#discussion_r1839101507


##
pinot-server/src/main/java/org/apache/pinot/server/starter/helix/HelixInstanceDataManager.java:
##
@@ -180,6 +181,24 @@ private void initInstanceDataDir(File instanceDataDir) {
 }
   }
 
+  @Override
+  public List getConsumerDirPaths() {
+String consumerDirPath = _instanceDataManagerConfig.getConsumerDir();
+
+if (consumerDirPath != null) {
+  return Collections.singletonList(new File(consumerDirPath));
+} else {

Review Comment:
   updated to use `RealtimeTableDataManager` for this logic which has a comment 
about this.



-- 
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-unsubscr...@pinot.apache.org

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


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



Re: [PR] Add instance level consumer dir usage metric [pinot]

2024-11-12 Thread via GitHub


SabrinaZhaozyf commented on code in PR #14430:
URL: https://github.com/apache/pinot/pull/14430#discussion_r1839099916


##
pinot-server/src/main/java/org/apache/pinot/server/starter/helix/HelixInstanceDataManager.java:
##
@@ -180,6 +181,24 @@ private void initInstanceDataDir(File instanceDataDir) {
 }
   }
 
+  @Override
+  public List getConsumerDirPaths() {
+String consumerDirPath = _instanceDataManagerConfig.getConsumerDir();
+
+if (consumerDirPath != null) {
+  return Collections.singletonList(new File(consumerDirPath));
+} else {
+  List consumerDirs = new ArrayList<>();
+  String instanceDataDir = _instanceDataManagerConfig.getInstanceDataDir();
+  for (String tableNameWithType : getAllTables()) {
+String dirPath = instanceDataDir + File.separator + tableNameWithType 
+ File.separator
++ RealtimeTableDataManager.CONSUMERS_DIR;
+consumerDirs.add(new File(dirPath));
+  }
+  return consumerDirs;

Review Comment:
   my concern with that is that the stored consumerDirs will need to be updated 
with table changes on the instance. Thus, relying on table data managers to 
fetch paths to avoid that.



-- 
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-unsubscr...@pinot.apache.org

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


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



Re: [PR] Add instance level consumer dir usage metric [pinot]

2024-11-12 Thread via GitHub


SabrinaZhaozyf commented on code in PR #14430:
URL: https://github.com/apache/pinot/pull/14430#discussion_r1839098791


##
pinot-server/src/main/java/org/apache/pinot/server/starter/helix/BaseServerStarter.java:
##
@@ -707,6 +711,23 @@ public void start()
 _serverQueriesDisabledTracker =
 new ServerQueriesDisabledTracker(_helixClusterName, _instanceId, 
_helixManager, serverMetrics);
 _serverQueriesDisabledTracker.start();
+
+// Add metrics for consumer directory usage
+serverMetrics.setOrUpdateGlobalGauge(ServerGauge.CONSUMER_DIR_USAGE, () -> 
{
+  List serverConsumerDirs = 
instanceDataManager.getConsumerDirPaths();
+  final long[] totalSize = {0};

Review Comment:
   Changed to long. Previously used another lambda there that used the array!



-- 
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-unsubscr...@pinot.apache.org

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


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



Re: [PR] Add instance level consumer dir usage metric [pinot]

2024-11-12 Thread via GitHub


jasperjiaguo commented on code in PR #14430:
URL: https://github.com/apache/pinot/pull/14430#discussion_r1838620339


##
pinot-server/src/main/java/org/apache/pinot/server/starter/helix/HelixInstanceDataManager.java:
##
@@ -180,6 +181,24 @@ private void initInstanceDataDir(File instanceDataDir) {
 }
   }
 
+  @Override
+  public List getConsumerDirPaths() {
+String consumerDirPath = _instanceDataManagerConfig.getConsumerDir();
+
+if (consumerDirPath != null) {
+  return Collections.singletonList(new File(consumerDirPath));
+} else {
+  List consumerDirs = new ArrayList<>();
+  String instanceDataDir = _instanceDataManagerConfig.getInstanceDataDir();
+  for (String tableNameWithType : getAllTables()) {
+String dirPath = instanceDataDir + File.separator + tableNameWithType 
+ File.separator
++ RealtimeTableDataManager.CONSUMERS_DIR;
+consumerDirs.add(new File(dirPath));
+  }
+  return consumerDirs;

Review Comment:
   should we store return value (consumerDirs) up so that we don't calculate 
these every time? We can also add logging to it if it is calculated just once.



-- 
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-unsubscr...@pinot.apache.org

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


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



Re: [PR] Add instance level consumer dir usage metric [pinot]

2024-11-12 Thread via GitHub


vvivekiyer commented on code in PR #14430:
URL: https://github.com/apache/pinot/pull/14430#discussion_r1838747910


##
pinot-server/src/main/java/org/apache/pinot/server/starter/helix/HelixInstanceDataManager.java:
##
@@ -180,6 +181,24 @@ private void initInstanceDataDir(File instanceDataDir) {
 }
   }
 
+  @Override
+  public List getConsumerDirPaths() {
+String consumerDirPath = _instanceDataManagerConfig.getConsumerDir();
+
+if (consumerDirPath != null) {
+  return Collections.singletonList(new File(consumerDirPath));
+} else {
+  List consumerDirs = new ArrayList<>();
+  String instanceDataDir = _instanceDataManagerConfig.getInstanceDataDir();
+  for (String tableNameWithType : getAllTables()) {
+String dirPath = instanceDataDir + File.separator + tableNameWithType 
+ File.separator

Review Comment:
   Will this also end up considering offline tables? Do we want to just do this 
for realtime tables ? If we are doing this for all tables, then it might just 
be enough to calculate the size of instanceDataDir?



##
pinot-server/src/main/java/org/apache/pinot/server/starter/helix/HelixInstanceDataManager.java:
##
@@ -180,6 +181,24 @@ private void initInstanceDataDir(File instanceDataDir) {
 }
   }
 
+  @Override
+  public List getConsumerDirPaths() {
+String consumerDirPath = _instanceDataManagerConfig.getConsumerDir();
+
+if (consumerDirPath != null) {
+  return Collections.singletonList(new File(consumerDirPath));
+} else {
+  List consumerDirs = new ArrayList<>();

Review Comment:
   This code block is repeating the logic in 
RealtimeTableDataManager.getConsumerDir right? Can we move that logic to a 
common place and reuse from both places? 
   
   Duplicating logic can result in the metrics breaking if this logic changes 
in`RealtimeTableDataManager.getConsumerDir`



-- 
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-unsubscr...@pinot.apache.org

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


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



Re: [PR] Add instance level consumer dir usage metric [pinot]

2024-11-12 Thread via GitHub


codecov-commenter commented on PR #14430:
URL: https://github.com/apache/pinot/pull/14430#issuecomment-2471353767

   ## 
[Codecov](https://app.codecov.io/gh/apache/pinot/pull/14430?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
 Report
   All modified and coverable lines are covered by tests :white_check_mark:
   > Project coverage is 0.00%. Comparing base 
[(`59551e4`)](https://app.codecov.io/gh/apache/pinot/commit/59551e45224f1535c4863fd577622b37366ccc97?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
 to head 
[(`b1d36e1`)](https://app.codecov.io/gh/apache/pinot/commit/b1d36e1d39f49b4b856aa52fba9862af44b12a1d?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
   > Report is 1313 commits behind head on master.
   
   > :exclamation:  There is a different number of reports uploaded between 
BASE (59551e4) and HEAD (b1d36e1). Click for more details.
   > 
   > HEAD has 53 uploads less than BASE
   >
   >| Flag | BASE (59551e4) | HEAD (b1d36e1) |
   >|--|--|--|
   >|integration|7|1|
   >|integration2|3|1|
   >|temurin|12|1|
   >|java-21|7|1|
   >|skip-bytebuffers-true|3|1|
   >|skip-bytebuffers-false|7|0|
   >|unittests|5|0|
   >|unittests1|2|0|
   >|java-11|5|0|
   >|unittests2|3|0|
   >|integration1|2|0|
   >|custom-integration1|2|0|
   >
   
   Additional details and impacted files
   
   
   ```diff
   @@  Coverage Diff  @@
   ## master   #14430   +/-   ##
   =
   - Coverage 61.75%0.00%   -61.76% 
   =
 Files  24363 -2433 
 Lines1332336   -133227 
 Branches  206360-20636 
   =
   - Hits  822740-82274 
   + Misses449116-44905 
   + Partials   60480 -6048 
   ```
   
   | 
[Flag](https://app.codecov.io/gh/apache/pinot/pull/14430/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
 | Coverage Δ | |
   |---|---|---|
   | 
[custom-integration1](https://app.codecov.io/gh/apache/pinot/pull/14430/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
 | `?` | |
   | 
[integration](https://app.codecov.io/gh/apache/pinot/pull/14430/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
 | `0.00% <ø> (-0.01%)` | :arrow_down: |
   | 
[integration1](https://app.codecov.io/gh/apache/pinot/pull/14430/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
 | `?` | |
   | 
[integration2](https://app.codecov.io/gh/apache/pinot/pull/14430/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
 | `0.00% <ø> (ø)` | |
   | 
[java-11](https://app.codecov.io/gh/apache/pinot/pull/14430/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
 | `?` | |
   | 
[java-21](https://app.codecov.io/gh/apache/pinot/pull/14430/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
 | `0.00% <ø> (-61.63%)` | :arrow_down: |
   | 
[skip-bytebuffers-false](https://app.codecov.io/gh/apache/pinot/pull/14430/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
 | `?` | |
   | 
[skip-bytebuffers-true](https://app.codecov.io/gh/apache/pinot/pull/14430/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
 | `0.00% <ø> (-27.73%)` | :arrow_down: |
   | 
[temurin](https://app.codecov.io/gh/apache/pinot/pull/14430/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
 | `0.00% <ø> (-61.76%)` | :arrow_down: |
   | 
[unittests](https://app.codecov.io/gh/apache/pinot/pull/14430/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
 | `?` | |
   | 
[unittests1](https://app.codecov.io/gh/apache/pinot/pull/14430/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
 | `?` | |
   | 
[unittests2](https://app.codecov.io/gh/apache/pinot/pull/14430/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
 | `?` | |
   
   Flags with carried for

Re: [PR] Add instance level consumer dir usage metric [pinot]

2024-11-12 Thread via GitHub


jasperjiaguo commented on code in PR #14430:
URL: https://github.com/apache/pinot/pull/14430#discussion_r1838620339


##
pinot-server/src/main/java/org/apache/pinot/server/starter/helix/HelixInstanceDataManager.java:
##
@@ -180,6 +181,24 @@ private void initInstanceDataDir(File instanceDataDir) {
 }
   }
 
+  @Override
+  public List getConsumerDirPaths() {
+String consumerDirPath = _instanceDataManagerConfig.getConsumerDir();
+
+if (consumerDirPath != null) {
+  return Collections.singletonList(new File(consumerDirPath));
+} else {
+  List consumerDirs = new ArrayList<>();
+  String instanceDataDir = _instanceDataManagerConfig.getInstanceDataDir();
+  for (String tableNameWithType : getAllTables()) {
+String dirPath = instanceDataDir + File.separator + tableNameWithType 
+ File.separator
++ RealtimeTableDataManager.CONSUMERS_DIR;
+consumerDirs.add(new File(dirPath));
+  }
+  return consumerDirs;

Review Comment:
   should we store returnb value (consumerDirs) up so that we don't calculate 
these every time? We can add logging to it if it is calculated just once.



##
pinot-server/src/main/java/org/apache/pinot/server/starter/helix/HelixInstanceDataManager.java:
##
@@ -180,6 +181,24 @@ private void initInstanceDataDir(File instanceDataDir) {
 }
   }
 
+  @Override
+  public List getConsumerDirPaths() {
+String consumerDirPath = _instanceDataManagerConfig.getConsumerDir();
+
+if (consumerDirPath != null) {
+  return Collections.singletonList(new File(consumerDirPath));
+} else {

Review Comment:
   this is when consuming and completed dir is mixed up right? could we add 
some comment to the code?



-- 
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-unsubscr...@pinot.apache.org

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


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



Re: [PR] Add instance level consumer dir usage metric [pinot]

2024-11-12 Thread via GitHub


sajjad-moradi commented on code in PR #14430:
URL: https://github.com/apache/pinot/pull/14430#discussion_r1838620812


##
pinot-server/src/main/java/org/apache/pinot/server/starter/helix/BaseServerStarter.java:
##
@@ -707,6 +711,23 @@ public void start()
 _serverQueriesDisabledTracker =
 new ServerQueriesDisabledTracker(_helixClusterName, _instanceId, 
_helixManager, serverMetrics);
 _serverQueriesDisabledTracker.start();
+
+// Add metrics for consumer directory usage
+serverMetrics.setOrUpdateGlobalGauge(ServerGauge.CONSUMER_DIR_USAGE, () -> 
{
+  List serverConsumerDirs = 
instanceDataManager.getConsumerDirPaths();
+  final long[] totalSize = {0};

Review Comment:
   why array? Single long is not enough?



##
pinot-server/src/main/java/org/apache/pinot/server/starter/helix/HelixInstanceDataManager.java:
##
@@ -180,6 +181,24 @@ private void initInstanceDataDir(File instanceDataDir) {
 }
   }
 
+  @Override
+  public List getConsumerDirPaths() {
+String consumerDirPath = _instanceDataManagerConfig.getConsumerDir();
+
+if (consumerDirPath != null) {
+  return Collections.singletonList(new File(consumerDirPath));
+} else {
+  List consumerDirs = new ArrayList<>();
+  String instanceDataDir = _instanceDataManagerConfig.getInstanceDataDir();
+  for (String tableNameWithType : getAllTables()) {
+String dirPath = instanceDataDir + File.separator + tableNameWithType 
+ File.separator
++ RealtimeTableDataManager.CONSUMERS_DIR;
+consumerDirs.add(new File(dirPath));
+  }
+  return consumerDirs;

Review Comment:
   My understanding is that if the consumerDir is not specified, the table is 
not mmapping the consuming segments (it uses direct offheap). So maybe we don't 
need the else part?!



-- 
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-unsubscr...@pinot.apache.org

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


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