Skip to content

Conversation

@ericm-db
Copy link
Contributor

@ericm-db ericm-db commented Aug 1, 2025

What changes were proposed in this pull request?

Follow up of #51708, addressing nits and test feedback

Why are the changes needed?

To conform with Spark style standards and make tests less flaky

Does this PR introduce any user-facing change?

No

How was this patch tested?

Unit tests

Was this patch authored or co-authored using generative AI tooling?

No

Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for making a quick followup, @ericm-db .

.version("4.1.0")
.timeConf(TimeUnit.MILLISECONDS)
.createWithDefaultString("1s")
.createWithDefaultString("0s")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you.

}
// Check for memory tracking - the continuous stream should trigger memory updates
var initialRocksDBMemory = 0L
eventually(timeout(Span(20, Seconds)), interval(Span(500, Millis))) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the change. Let's see the CI result.

@dongjoon-hyun dongjoon-hyun changed the title [SPARK-53001][FOLLOW-UP] Integrate RocksDB Memory Usage with the Unified Memory Manager [SPARK-53001][CORE][SQL][FOLLOW-UP] Disable spark.memory.unmanagedMemoryPollingInterval by default Aug 1, 2025
@dongjoon-hyun dongjoon-hyun changed the title [SPARK-53001][CORE][SQL][FOLLOW-UP] Disable spark.memory.unmanagedMemoryPollingInterval by default [SPARK-53001][CORE][SQL][FOLLOW-UP] Disable spark.memory.unmanagedMemoryPollingInterval by default Aug 1, 2025
@ericm-db
Copy link
Contributor Author

ericm-db commented Aug 1, 2025

@dongjoon-hyun This job keeps timing out, but the tests are passing - I think there are simply too many tests to run here.

@dongjoon-hyun
Copy link
Member

@dongjoon-hyun This job keeps timing out, but the tests are passing - I think there are simply too many tests to run here.

Ya, you are correct. I'm trying to rebalance the CI via the following.

Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, LGTM.

I verified this manually.

$ build/sbt "sql/testOnly *.RocksDBStateStoreIntegrationSuite"
...
[info] Run completed in 19 seconds, 890 milliseconds.
[info] Total number of tests run: 29
[info] Suites: completed 1, aborted 0
[info] Tests: succeeded 29, failed 0, canceled 0, ignored 0, pending 0
[info] All tests passed.
[success] Total time: 83 s (01:23), completed Aug 1, 2025, 1:56:13 PM

@ericm-db
Copy link
Contributor Author

ericm-db commented Aug 1, 2025

Great, thanks for the help and the feedback here @dongjoon-hyun !

@dongjoon-hyun
Copy link
Member

Merged to master. Thank YOU, @ericm-db .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants