You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@gluten.apache.org by "luis4a0 (via GitHub)" <gi...@apache.org> on 2026/05/19 06:21:12 UTC
[PR] [CORE] Add customMetrics extension point to ShuffleWriterMetrics for backend-specific shuffle stats [gluten]
luis4a0 opened a new pull request, #12114:
URL: https://github.com/apache/gluten/pull/12114
PLACEHOLDER will be replaced via gh api
--
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@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [CORE] Add customMetrics extension point to ShuffleWriterMetrics for backend-specific shuffle stats [gluten]
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn commented on code in PR #12114:
URL: https://github.com/apache/gluten/pull/12114#discussion_r3265060815
##########
gluten-arrow/src/main/java/org/apache/gluten/vectorized/GlutenSplitResult.java:
##########
@@ -122,4 +139,32 @@ public double getAvgDictionaryFields() {
public long getDictionarySize() {
return dictionarySize;
}
+
+ /**
+ * Backend-specific shuffle writer metrics, keyed by `<Backend>.<Family>.<Stat>`. The map
+ * preserves the iteration order JNI marshalled, but callers should treat the map as unordered.
+ * Returns an empty map if the native side did not populate any custom metrics (e.g. older Gluten
+ * libs, or backends that don't yet emit any). The returned map is unmodifiable.
+ */
+ public Map<String, Long> getCustomMetrics() {
+ Map<String, Long> cached = customMetricsCache;
+ if (cached != null) {
+ return cached;
+ }
+ synchronized (this) {
+ if (customMetricsCache != null) {
+ return customMetricsCache;
+ }
+ if (customMetricsKeys == null || customMetricsKeys.length == 0) {
Review Comment:
`getCustomMetrics()` trusts `customMetricsValues.length == customMetricsKeys.length`. If a future producer ships mismatched arrays, the AIOOBE inside the synchronized block leaves the volatile cache field unassigned — subsequent calls re-enter and AIOOBE again. Suggest a `Preconditions.checkArgument(...)` at the top + one suite 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-unsubscribe@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [CORE] Add customMetrics extension point to ShuffleWriterMetrics for backend-specific shuffle stats [gluten]
Posted by "luis4a0 (via GitHub)" <gi...@apache.org>.
luis4a0 commented on code in PR #12114:
URL: https://github.com/apache/gluten/pull/12114#discussion_r3265097543
##########
backends-velox/src/main/scala/org/apache/spark/shuffle/ColumnarShuffleWriter.scala:
##########
@@ -258,6 +258,18 @@ class ColumnarShuffleWriter[K, V](
dep.metrics("dataSize").add(splitResult.getRawPartitionLengths.sum)
dep.metrics("compressTime").add(splitResult.getTotalCompressTime)
dep.metrics("peakBytes").add(splitResult.getPeakBytes)
+ // Backend-specific custom metrics (see GlutenSplitResult.getCustomMetrics).
+ // Only entries whose key is pre-registered in `dep.metrics` are recorded;
+ // unknown keys are silently dropped so that the native side can ship new
+ // metrics ahead of the Spark-side `VeloxMetricsApi` registration without
+ // breaking older builds.
+ splitResult.getCustomMetrics.forEach {
+ (key, value) =>
+ val m = dep.metrics.get(key)
+ if (m.isDefined) {
+ m.get.add(value)
Review Comment:
Good catch — fully agreed about the parity gap across the 3 writer entry points. This particular concern is now moot because, per offline reviewer feedback, we dropped the second commit entirely (the one that wired `getCustomMetrics()` into `ColumnarShuffleWriter.scala`). The Scala consumer side will come back as small follow-up PRs once specific metrics have proven useful, and your `applyCustomMetrics(splitResult, dep)` helper is exactly the right shape for the first such follow-up — I'll factor it that way to keep all 3 writers in sync from day one.
Will add a TODO note next to `getCustomMetrics()` in `GlutenSplitResult` so the helper-shape requirement is captured for whoever picks up the first per-metric follow-up.
--
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@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [CORE] Add customMetrics extension point to ShuffleWriterMetrics for backend-specific shuffle stats [gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #12114:
URL: https://github.com/apache/gluten/pull/12114#issuecomment-4880289630
This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 10 days.
--
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@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [CORE] Add customMetrics extension point to ShuffleWriterMetrics for backend-specific shuffle stats [gluten]
Posted by "luis4a0 (via GitHub)" <gi...@apache.org>.
luis4a0 commented on code in PR #12114:
URL: https://github.com/apache/gluten/pull/12114#discussion_r3265097543
##########
backends-velox/src/main/scala/org/apache/spark/shuffle/ColumnarShuffleWriter.scala:
##########
@@ -258,6 +258,18 @@ class ColumnarShuffleWriter[K, V](
dep.metrics("dataSize").add(splitResult.getRawPartitionLengths.sum)
dep.metrics("compressTime").add(splitResult.getTotalCompressTime)
dep.metrics("peakBytes").add(splitResult.getPeakBytes)
+ // Backend-specific custom metrics (see GlutenSplitResult.getCustomMetrics).
+ // Only entries whose key is pre-registered in `dep.metrics` are recorded;
+ // unknown keys are silently dropped so that the native side can ship new
+ // metrics ahead of the Spark-side `VeloxMetricsApi` registration without
+ // breaking older builds.
+ splitResult.getCustomMetrics.forEach {
+ (key, value) =>
+ val m = dep.metrics.get(key)
+ if (m.isDefined) {
+ m.get.add(value)
Review Comment:
Good catch — fully agreed about the parity gap across the 3 writer entry points. This particular concern is now moot because, per offline reviewer feedback, we dropped the second commit entirely (the one that wired `getCustomMetrics()` into `ColumnarShuffleWriter.scala`). The Scala consumer side will come back as small follow-up PRs once specific metrics have proven useful, and your `applyCustomMetrics(splitResult, dep)` helper is exactly the right shape for the first such follow-up — I'll factor it that way to keep all 3 writers in sync from day one.
Will add a TODO note next to `getCustomMetrics()` in `GlutenSplitResult` so the helper-shape requirement is captured for whoever picks up the first per-metric follow-up.
##########
gluten-arrow/src/main/java/org/apache/gluten/vectorized/GlutenSplitResult.java:
##########
@@ -122,4 +139,32 @@ public double getAvgDictionaryFields() {
public long getDictionarySize() {
return dictionarySize;
}
+
+ /**
+ * Backend-specific shuffle writer metrics, keyed by `<Backend>.<Family>.<Stat>`. The map
+ * preserves the iteration order JNI marshalled, but callers should treat the map as unordered.
+ * Returns an empty map if the native side did not populate any custom metrics (e.g. older Gluten
+ * libs, or backends that don't yet emit any). The returned map is unmodifiable.
+ */
+ public Map<String, Long> getCustomMetrics() {
+ Map<String, Long> cached = customMetricsCache;
+ if (cached != null) {
+ return cached;
+ }
+ synchronized (this) {
+ if (customMetricsCache != null) {
+ return customMetricsCache;
+ }
+ if (customMetricsKeys == null || customMetricsKeys.length == 0) {
Review Comment:
Excellent point. Fixed in 6e6d98d. Added an explicit length-match check at the top of the synchronized block that throws `IllegalStateException` mentioning both lengths (and the case where values is null while keys is non-empty), so the producer-side bug is unambiguous and the cache field never gets assigned to a partial map. Two new `GlutenSplitResultSuite` cases:
- `fails loudly on mismatched key/value array lengths` — also asserts the second call still throws (the cache field stays null on the failure path)
- `fails loudly when values array is null but keys is non-empty`
Both pass locally; full suite is now 8/8.
--
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@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [CORE] Add customMetrics extension point to ShuffleWriterMetrics for backend-specific shuffle stats [gluten]
Posted by "luis4a0 (via GitHub)" <gi...@apache.org>.
luis4a0 commented on code in PR #12114:
URL: https://github.com/apache/gluten/pull/12114#discussion_r3265097747
##########
gluten-arrow/src/main/java/org/apache/gluten/vectorized/GlutenSplitResult.java:
##########
@@ -122,4 +139,32 @@ public double getAvgDictionaryFields() {
public long getDictionarySize() {
return dictionarySize;
}
+
+ /**
+ * Backend-specific shuffle writer metrics, keyed by `<Backend>.<Family>.<Stat>`. The map
+ * preserves the iteration order JNI marshalled, but callers should treat the map as unordered.
+ * Returns an empty map if the native side did not populate any custom metrics (e.g. older Gluten
+ * libs, or backends that don't yet emit any). The returned map is unmodifiable.
+ */
+ public Map<String, Long> getCustomMetrics() {
+ Map<String, Long> cached = customMetricsCache;
+ if (cached != null) {
+ return cached;
+ }
+ synchronized (this) {
+ if (customMetricsCache != null) {
+ return customMetricsCache;
+ }
+ if (customMetricsKeys == null || customMetricsKeys.length == 0) {
Review Comment:
Excellent point. Fixed in 722fa45. Added an explicit length-match check at the top of the synchronized block that throws `IllegalStateException` mentioning both lengths (and the case where values is null while keys is non-empty), so the producer-side bug is unambiguous and the cache field never gets assigned to a partial map. Two new `GlutenSplitResultSuite` cases:
- `fails loudly on mismatched key/value array lengths` — also asserts the second call still throws (the cache field stays null on the failure path)
- `fails loudly when values array is null but keys is non-empty`
Both pass locally; full suite is now 8/8.
--
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@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [CORE] Add customMetrics extension point to ShuffleWriterMetrics for backend-specific shuffle stats [gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #12114: [CORE] Add customMetrics extension point to ShuffleWriterMetrics for backend-specific shuffle stats
URL: https://github.com/apache/gluten/pull/12114
--
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@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [CORE] Add customMetrics extension point to ShuffleWriterMetrics for backend-specific shuffle stats [gluten]
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn commented on code in PR #12114:
URL: https://github.com/apache/gluten/pull/12114#discussion_r3265040395
##########
backends-velox/src/main/scala/org/apache/spark/shuffle/ColumnarShuffleWriter.scala:
##########
@@ -258,6 +258,18 @@ class ColumnarShuffleWriter[K, V](
dep.metrics("dataSize").add(splitResult.getRawPartitionLengths.sum)
dep.metrics("compressTime").add(splitResult.getTotalCompressTime)
dep.metrics("peakBytes").add(splitResult.getPeakBytes)
+ // Backend-specific custom metrics (see GlutenSplitResult.getCustomMetrics).
+ // Only entries whose key is pre-registered in `dep.metrics` are recorded;
+ // unknown keys are silently dropped so that the native side can ship new
+ // metrics ahead of the Spark-side `VeloxMetricsApi` registration without
+ // breaking older builds.
+ splitResult.getCustomMetrics.forEach {
+ (key, value) =>
+ val m = dep.metrics.get(key)
+ if (m.isDefined) {
+ m.get.add(value)
Review Comment:
`splitResult.getCustomMetrics()` is consumed here but not in `VeloxCelebornColumnarShuffleWriter.scala:101+` or `VeloxUniffleColumnarShuffleWriter.java:232`. RSS users won't see new `Velox.InputEncoding.*` metrics. Suggest a shared `applyCustomMetrics(splitResult, dep)` helper called from all three writers.
--
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@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org
Re: [PR] [CORE] Add customMetrics extension point to ShuffleWriterMetrics for backend-specific shuffle stats [gluten]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #12114:
URL: https://github.com/apache/gluten/pull/12114#issuecomment-4976181906
This PR was auto-closed because it has been stalled for 10 days with no activity. Please feel free to reopen if it is still valid. Thanks.
--
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@gluten.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@gluten.apache.org
For additional commands, e-mail: commits-help@gluten.apache.org