Skip to content

[SPARK-43056][SS] RocksDB state store commit should continue background work only if its paused#40696

Closed
anishshri-db wants to merge 2 commits intoapache:masterfrom
anishshri-db:task/SPARK-43056
Closed

[SPARK-43056][SS] RocksDB state store commit should continue background work only if its paused#40696
anishshri-db wants to merge 2 commits intoapache:masterfrom
anishshri-db:task/SPARK-43056

Conversation

@anishshri-db
Copy link
Contributor

What changes were proposed in this pull request?

RocksDB state store commit should continue background work in finally only if its paused

Why are the changes needed?

If an exception is thrown earlier in the commit sequence before background work is paused, we fail with an exception in the finally clause.

Does this PR introduce any user-facing change?

No

How was this patch tested?

Ran unit tests

StateStoreIntegrationSuite
[info] Run completed in 16 seconds, 131 milliseconds.
[info] Total number of tests run: 5
[info] Suites: completed 1, aborted 0
[info] Tests: succeeded 5, failed 0, canceled 0, ignored 0, pending 0
[info] All tests passed.
StateStoreSuite
[info] Run completed in 1 minute, 18 seconds.                                                                                                                                                                                                                                              [info] Total number of tests run: 73
[info] Suites: completed 1, aborted 0
[info] Tests: succeeded 73, failed 0, canceled 0, ignored 0, pending 0
[info] All tests passed.

With simulated exception and without the fix, the code crashes with stack trace below. With the fix, we don't see the issue in the finally block.

  org.rocksdb.RocksDBException: InvalidArgument
  at org.rocksdb.RocksDB.continueBackgroundWork(Native Method)
  at org.rocksdb.RocksDB.continueBackgroundWork(RocksDB.java:3611)
  at org.apache.spark.sql.execution.streaming.state.RocksDB.commit(RocksDB.scala:421)

@anishshri-db anishshri-db changed the title [SPARK-43056] RocksDB state store commit should continue b/ground work only if its paused [SPARK-43056][SS] RocksDB state store commit should continue b/ground work only if its paused Apr 7, 2023
@anishshri-db
Copy link
Contributor Author

@HeartSaVioR - PTAL. Thanks

Copy link
Contributor

@HeartSaVioR HeartSaVioR left a comment

Choose a reason for hiding this comment

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

+1 Nice finding!

@HeartSaVioR
Copy link
Contributor

CI fails with odd errors.
@anishshri-db Could you please rebase so that CI is retriggered? If the new trial fails again, maybe good to post to dev@ and see whether someone encountered this before, and/or someone is willing to volunteer to fix if that's not a transient issue.

@anishshri-db
Copy link
Contributor Author

Could you please rebase so that CI is retriggered? If the new trial fails again, maybe good to post to dev@ and see whether someone encountered this before, and/or someone is willing to volunteer to fix if that's not a transient issue.

Seems like a known issue someone already posted to dev channel - sbt/sbt#7202

@anishshri-db
Copy link
Contributor Author

@HeartSaVioR - all tests passed. Please merge when you get a chance. Thx

@dongjoon-hyun dongjoon-hyun changed the title [SPARK-43056][SS] RocksDB state store commit should continue b/ground work only if its paused [SPARK-43056][SS] RocksDB state store commit should continue background work only if its paused Apr 10, 2023
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.

@HeartSaVioR
Copy link
Contributor

Missed the notification. Thanks! Merging to master.

Kimahriman pushed a commit to Kimahriman/spark that referenced this pull request Jun 20, 2023
…nd work only if its paused

### What changes were proposed in this pull request?
RocksDB state store commit should continue background work in finally only if its paused

### Why are the changes needed?
If an exception is thrown earlier in the commit sequence before background work is paused, we fail with an exception in the finally clause.

### Does this PR introduce _any_ user-facing change?
No

### How was this patch tested?
Ran unit tests
```
StateStoreIntegrationSuite
[info] Run completed in 16 seconds, 131 milliseconds.
[info] Total number of tests run: 5
[info] Suites: completed 1, aborted 0
[info] Tests: succeeded 5, failed 0, canceled 0, ignored 0, pending 0
[info] All tests passed.
```

```
StateStoreSuite
[info] Run completed in 1 minute, 18 seconds.                                                                                                                                                                                                                                              [info] Total number of tests run: 73
[info] Suites: completed 1, aborted 0
[info] Tests: succeeded 73, failed 0, canceled 0, ignored 0, pending 0
[info] All tests passed.
```

With simulated exception and without the fix, the code crashes with stack trace below. With the fix, we don't see the issue in the finally block.
```
  org.rocksdb.RocksDBException: InvalidArgument
  at org.rocksdb.RocksDB.continueBackgroundWork(Native Method)
  at org.rocksdb.RocksDB.continueBackgroundWork(RocksDB.java:3611)
  at org.apache.spark.sql.execution.streaming.state.RocksDB.commit(RocksDB.scala:421)
```

Closes apache#40696 from anishshri-db/task/SPARK-43056.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants