[SPARK-48208][SS] Skip providing memory usage metrics from RocksDB if bounded memory usage is enabled #46491
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…nded memory usage is enabled
anishshri-db
changed the title
|
@HeartSaVioR - could you PTAL, thx ! |
HeartSaVioR
reviewed
May 9, 2024
| // running on the same node and account the usage to this single cache. In this case, its not | ||
| // possible to provide partition level or query level memory usage. | ||
| val memoryUsage = if (conf.boundedMemoryUsage) { | ||
| 0L |
There was a problem hiding this comment.
Could you please try with -1L? I'd say let's distinguish without doubt if possible.
There was a problem hiding this comment.
Tried this, but it seems the progress metrics will still convert this to 0
"stateOperators" : [ {
"operatorName" : "stateStoreSave",
"numRowsTotal" : 3,
"numRowsUpdated" : 3,
"allUpdatesTimeMs" : 10,
"numRowsRemoved" : 0,
"allRemovalsTimeMs" : 0,
"commitTimeMs" : 211,
"memoryUsedBytes" : 0,
It seems the lowest allowed value for the SQLMetric will still be interpreted as 0.
We seem to do the same thing for reporting numRowsTotal if the trackingTotalNumRows flag is disabled as well
There was a problem hiding this comment.
Ah OK. Let's leave it as it is. Probably I was also struggled about this and forgot.
|
Thanks! Merging to master. |
JacobZheng0927
pushed a commit
to JacobZheng0927/spark
that referenced
this pull request
May 11, 2024
… bounded memory usage is enabled
### What changes were proposed in this pull request?
Skip providing memory usage metrics from RocksDB if bounded memory usage is enabled
### Why are the changes needed?
Without this, we are providing memory usage that is the max usage per node at a partition level.
For eg - if we report this
```
"allRemovalsTimeMs" : 93,
"commitTimeMs" : 32240,
"memoryUsedBytes" : 15956211724278,
"numRowsDroppedByWatermark" : 0,
"numShufflePartitions" : 200,
"numStateStoreInstances" : 200,
```
We have 200 partitions in this case.
So the memory usage per partition / state store would be ~78GB. However, this node has 256GB memory total and we have 2 such nodes. We have configured our cluster to use 30% of available memory on each node for RocksDB which is ~77GB.
So the memory being reported here is actually per node rather than per partition which could be confusing for users.
### Does this PR introduce _any_ user-facing change?
No - only a metrics reporting change
### How was this patch tested?
Added unit tests
```
[info] Run completed in 10 seconds, 878 milliseconds.
[info] Total number of tests run: 24
[info] Suites: completed 1, aborted 0
[info] Tests: succeeded 24, failed 0, canceled 0, ignored 0, pending 0
[info] All tests passed.
```
### Was this patch authored or co-authored using generative AI tooling?
No
Closes apache#46491 from anishshri-db/task/SPARK-48208.
Authored-by: Anish Shrigondekar <anish.shrigondekar@databricks.com>
Signed-off-by: Jungtaek Lim <kabhwan.opensource@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
Skip providing memory usage metrics from RocksDB if bounded memory usage is enabled
Why are the changes needed?
Without this, we are providing memory usage that is the max usage per node at a partition level.
For eg - if we report this
We have 200 partitions in this case.
So the memory usage per partition / state store would be ~78GB. However, this node has 256GB memory total and we have 2 such nodes. We have configured our cluster to use 30% of available memory on each node for RocksDB which is ~77GB.
So the memory being reported here is actually per node rather than per partition which could be confusing for users.
Does this PR introduce any user-facing change?
No - only a metrics reporting change
How was this patch tested?
Added unit tests
Was this patch authored or co-authored using generative AI tooling?
No