[SPARK-38909][CORE][YARN] Encapsulate `LevelDB` used to store remote/external shuffle state as `DB` #36200

LuciferYang · 2022-04-14T13:05:37Z

What changes were proposed in this pull request?

ExternalShuffleBlockResolver, YarnShuffleService and RemoteBlockPushResolver use LevelDB directly, this is not conducive to extending the use of RocksDB in this scenario. This pr is encapsulated for expansibility. It will be the pre-work of SPARK-38888, the main change as follows:

Introduce DB interface and implementation of corresponding LevelDB now, RocksDB implementation will be involved in the future
Introduce DBBackend enum and new config SHUFFLE_SERVICE_DB_BACKEND to specify a disk-based store used in shuffle service local db, only LEVELDB is supported now, RocksDB implementation will be involved in the future
Introduce DBIterator to traverse the items in the DB, only LevelDBIterator is supported now, RocksDBIterator implementation will be involved in the future
Introduce DBProvider to initialization DB, it is only used to generate LevelDB now, RocksDB implementation will be involved in the future
Change StoreVersion to top-level class, it will be reused by RocksDB implementation
Replace the directly use of LevelDB in ExternalShuffleBlockResolver, YarnShuffleService and RemoteBlockPushResolver with DB
fix corresponding UTs and test tools

Why are the changes needed?

This is pre work of SPARK-38888

Does this PR introduce any user-facing change?

Add new config SHUFFLE_SERVICE_DB_BACKEND(spark.shuffle.service.db.backend), the new config use to use to specify a disk-based store used in shuffle service local db. Only LEVELDB is supported after this pr.

How was this patch tested?

Pass GA

mridulm · 2022-08-09T04:05:24Z

@dongjoon-hyun

Actually, instead of providing wrapper layer, the final goal is the removal of LevelDB in favor of RocksDB.

I am sure in future, we might have another SPIP to remove RocksDB in favor of something else :-)
If we can abstract it away, the future evolution might be easier !

mridulm

Took a pass through it, looks mostly fine to me - will go over it again later this week.
But will let @dongjoon-hyun drive this review - since he is more close to the overall change.

common/network-common/src/main/java/org/apache/spark/network/shuffledb/LevelDBIterator.java

common/network-common/src/main/java/org/apache/spark/network/util/DBProvider.java

common/network-yarn/src/main/java/org/apache/spark/network/yarn/YarnShuffleService.java

mridulm · 2022-08-09T05:33:27Z

nit: Btw, from a completeness point of view, DB.get makes sense to include :-)

common/network-common/src/main/java/org/apache/spark/network/shuffledb/LevelDB.java

LuciferYang · 2022-08-09T05:52:27Z

nit: Btw, from a completeness point of view, DB.get makes sense to include :-)

OK ~

This reverts commit ee1f283.

LuciferYang · 2022-08-15T04:00:09Z

Also cc @tgravescs do you have time to help review this pr? Thanks ~

mridulm

Looks good to me, but would be good to have more eyes on this.

common/network-common/src/main/java/org/apache/spark/network/shuffledb/DB.java

common/network-common/src/main/java/org/apache/spark/network/shuffledb/DBBackend.java

common/network-common/src/main/java/org/apache/spark/network/shuffledb/StoreVersion.java

common/network-yarn/src/main/java/org/apache/spark/network/yarn/YarnShuffleService.java

core/src/main/scala/org/apache/spark/internal/config/package.scala

tgravescs · 2022-08-18T14:16:56Z

changes look fine to me

mridulm · 2022-08-18T15:01:02Z

Merged to master.
Thanks for working on this @LuciferYang !
Thanks for reviews @dongjoon-hyun, @zhouyejoe and @tgravescs :-)

LuciferYang · 2022-08-18T15:13:38Z

Thanks @mridulm @tgravescs @zhouyejoe @dongjoon-hyun !!!
I will do the RocksDB related work next week due to some other work needs to be completed this week ~

Ngone51 · 2022-08-24T11:58:21Z

common/network-common/src/main/java/org/apache/spark/network/shuffledb/DB.java

+ * The local KV storage used to persist the shuffle state,
+ * the implementations may include LevelDB, RocksDB, etc.
+ */
+public interface DB extends Closeable {


Is this a developer API or user API? I didn't see the custom DB could be plugged in this PR.

developer API. The implementation of RocksDB is under review: #37610,

So could you add annotation @DeveloperApi for it?

We do not support users plugging in a different DB instance - it comes from within spark code.
Supported DB's come from DBBackend and DBProvider.initDB has an explicit switch for the supported types.
Wont be @DeveloperAPI

@mridulm It's a public interface. I think it should either be a user API or developer API, no?

hmm... should we use @Private as kvstore.KVStore?

spark/common/kvstore/src/main/java/org/apache/spark/util/kvstore/KVStore.java

Lines 65 to 66 in 52cd037

@Private

public interface KVStore extends Closeable {

The configuration is public @Ngone51, not the implementation itself.
To put it differently, there is no way for users to leverage any of this - unless they modify spark code.

I am fine with marking it as @Private if we need to make the intent clearer.

It is the same for most submodules in common/* - except probably for common/kvstore, where Marcello marked the interfaces as @Private :-)

@mridulm @LuciferYang Thanks.

Ngone51 · 2022-08-24T12:31:29Z

...ork-shuffle/src/main/java/org/apache/spark/network/shuffle/ExternalShuffleBlockResolver.java

+      logger.info("Configured {} as {} and actually used value {}",
+        Constants.SHUFFLE_SERVICE_DB_BACKEND, dbBackendName, dbBackend);
+    }
+    db = DBProvider.initDB(dbBackend, this.registeredExecutorFile, CURRENT_VERSION, mapper);


Ideally, this should be put inside the if block?

Yes, you're right. I'll give a follow-up to fix this

Need change as

if (registeredExecutorFile != null) { .... db = DBProvider.initDB(dbBackend, this.registeredExecutorFile, CURRENT_VERSION, mapper); } else { db = null; }

due to db is defined as final, and when 'registeredExecutorFile' is null, 'DBProvider.initDB' will return null directly . So does DBProvider.initDB still need to be put inside the if block ?

If so, I think if (registeredExecutorFile != null) is actually useless here..

DBProvider.initDB handles null registeredExecutorFile as input.
Given db is a final variable, it is better to update it at a single place, and not across if/else

I'm trying to delete if (registeredExecutorFile != null) condition

The current code in #37648 looks ok to me.

Ngone51 · 2022-08-24T12:36:24Z

core/src/main/scala/org/apache/spark/deploy/ExternalShuffleService.scala

+      val shuffleDBName = sparkConf.get(config.SHUFFLE_SERVICE_DB_BACKEND)
+      val dBBackend = DBBackend.byName(shuffleDBName)
+      logInfo(s"Configured ${config.SHUFFLE_SERVICE_DB_BACKEND.key} as $shuffleDBName " +
+        s"and actually used value ${dBBackend.name()} ")


It sounds like we may use a different db backend regardless of the user configured value in some cases?

Sorry, because the DBBackend.byName method has changed during cr, this log is outdated. I will fix it in the follow-up also.

…ated to shuffle state db ### What changes were proposed in this pull request? This is a followup of #36200, the main change of this pr as follows: - Make `DB` and `DBProvider` as Private to make the API intent clearer - Delete `remove` method from `DBProvider` witch is unnecessary `Override` - Remove the useless null check condition from `ExternalShuffleBlockResolver` and `RemoteBlockPushResolver`, fix related sutes - Correction log print content ### Why are the changes needed? Fix new comments after #36200 merged. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? Pass All GitHub Actions Closes #37648 from LuciferYang/SPARK-38909-followup. Authored-by: yangjie01 <yangjie01@baidu.com> Signed-off-by: Mridul Muralidharan <mridul<at>gmail.com>

LuciferYang added 8 commits April 14, 2022 12:02

Introduce LocalDB

181a597

refactor ShuffleTestAccessor

1e5654a

refactor ShuffleTestAccessor

a4ae04c

refactor ShuffleTestAccessor

29a7a1e

reformat import

eacc159

reformat import

ea21178

reformat import

f57578e

remove ;

ad5b74f

github-actions bot added CORE YARN labels Apr 14, 2022

LuciferYang marked this pull request as draft April 14, 2022 13:11

LuciferYang changed the title ~~[DON'T MERGE] Encapsulate LevelDB used by ExternalShuffleBlockResolver and YarnShuffleService as LocalDB~~ [DON'T MERGE][CORE][YARN] Encapsulate LevelDB used by ExternalShuffleBlockResolver and YarnShuffleService as LocalDB Apr 15, 2022

add APL

4d5739b

LuciferYang changed the title ~~[DON'T MERGE][CORE][YARN] Encapsulate LevelDB used by ExternalShuffleBlockResolver and YarnShuffleService as LocalDB~~ [SPARK-38909][CORE][YARN] Encapsulate LevelDB used by ExternalShuffleBlockResolver and YarnShuffleService as LocalDB Apr 15, 2022

LuciferYang added 4 commits April 15, 2022 10:50

Merge branch 'master' of github.com:apache/spark into local-db

610e04b

Merge branch 'upmaster' into local-db

c66c483

rename

6e0a6f7

rename

9ba181b

LuciferYang changed the title ~~[SPARK-38909][CORE][YARN] Encapsulate LevelDB used by ExternalShuffleBlockResolver and YarnShuffleService as LocalDB~~ [SPARK-38909][CORE][YARN] Encapsulate LevelDB used by ExternalShuffleBlockResolver and YarnShuffleService as DB Apr 24, 2022

LuciferYang marked this pull request as ready for review April 24, 2022 09:55

LuciferYang added 10 commits April 27, 2022 19:10

Merge branch 'upmaster' into local-db

c80024b

Merge branch 'master' of github.com:apache/spark into local-db

1c3f09d

Merge branch 'upmaster' into local-db

f03ea29

Merge branch 'upmaster' into local-db

ecba36a

Merge branch 'upmaster' into local-db

23753f5

Merge branch 'upmaster' into local-db

4630651

Merge branch 'upmaster' into local-db

712f99d

Merge branch 'upmaster' into local-db

01a6da1

Merge branch 'upmaster' into local-db

8e11695

Merge branch 'upmaster' into local-db

9c553a8

add config for DBBackend

f8cb374

LuciferYang force-pushed the local-db branch from ed3a329 to f8cb374 Compare August 8, 2022 15:49

pass by config

35bcced

Merge branch 'upmaster' into local-db

2dc68c3

mridulm reviewed Aug 9, 2022

View reviewed changes

remove DB.get method

ee1f283

LuciferYang commented Aug 9, 2022

View reviewed changes

common/network-common/src/main/java/org/apache/spark/network/shuffledb/LevelDB.java Outdated Show resolved Hide resolved

LuciferYang added 2 commits August 9, 2022 13:38

fix comments LevelDBIterator

5e856f6

add dBBackend.fileName

9590e97

LuciferYang added 4 commits August 9, 2022 13:52

Revert "remove DB.get method"

b299c55

This reverts commit ee1f283.

remove unnecessary RuntimeEexception throws declaration

94a2831

fix format

d4ce629

IllegalArgumentException for DBProvider

9889de3

mridulm approved these changes Aug 15, 2022

View reviewed changes

tgravescs reviewed Aug 15, 2022

View reviewed changes

add comments

abb0a58

mridulm closed this in 892a45f Aug 18, 2022

Ngone51 reviewed Aug 24, 2022

View reviewed changes

LuciferYang mentioned this pull request Aug 24, 2022

[SPARK-38909][BUILD][CORE][YARN][FOLLOWUP] Make some code cleanup related to shuffle state db #37648

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-38909][CORE][YARN] Encapsulate `LevelDB` used to store remote/external shuffle state as `DB` #36200

[SPARK-38909][CORE][YARN] Encapsulate `LevelDB` used to store remote/external shuffle state as `DB` #36200

LuciferYang commented Apr 14, 2022 •

edited

mridulm commented Aug 9, 2022

mridulm left a comment •

edited

mridulm commented Aug 9, 2022 •

edited

LuciferYang commented Aug 9, 2022

LuciferYang commented Aug 15, 2022

mridulm left a comment

tgravescs commented Aug 18, 2022

mridulm commented Aug 18, 2022

LuciferYang commented Aug 18, 2022

Ngone51 Aug 24, 2022

LuciferYang Aug 24, 2022

Ngone51 Aug 25, 2022

LuciferYang Aug 25, 2022

mridulm Aug 26, 2022 •

edited

Ngone51 Aug 26, 2022

LuciferYang Aug 26, 2022

mridulm Aug 26, 2022 •

edited

Ngone51 Sep 2, 2022

Ngone51 Aug 24, 2022

LuciferYang Aug 24, 2022

LuciferYang Aug 24, 2022 •

edited

Ngone51 Aug 26, 2022

LuciferYang Aug 26, 2022

mridulm Aug 26, 2022 •

edited

LuciferYang Aug 26, 2022

mridulm Aug 26, 2022 •

edited

Ngone51 Aug 24, 2022

LuciferYang Aug 24, 2022

[SPARK-38909][CORE][YARN] Encapsulate LevelDB used to store remote/external shuffle state as DB #36200

[SPARK-38909][CORE][YARN] Encapsulate LevelDB used to store remote/external shuffle state as DB #36200

Conversation

LuciferYang commented Apr 14, 2022 • edited

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

mridulm commented Aug 9, 2022

mridulm left a comment • edited

Choose a reason for hiding this comment

mridulm commented Aug 9, 2022 • edited

LuciferYang commented Aug 9, 2022

LuciferYang commented Aug 15, 2022

mridulm left a comment

Choose a reason for hiding this comment

tgravescs commented Aug 18, 2022

mridulm commented Aug 18, 2022

LuciferYang commented Aug 18, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mridulm Aug 26, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mridulm Aug 26, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LuciferYang Aug 24, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mridulm Aug 26, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mridulm Aug 26, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

[SPARK-38909][CORE][YARN] Encapsulate `LevelDB` used to store remote/external shuffle state as `DB` #36200

[SPARK-38909][CORE][YARN] Encapsulate `LevelDB` used to store remote/external shuffle state as `DB` #36200

LuciferYang commented Apr 14, 2022 •

edited

mridulm left a comment •

edited

mridulm commented Aug 9, 2022 •

edited

mridulm Aug 26, 2022 •

edited

mridulm Aug 26, 2022 •

edited

LuciferYang Aug 24, 2022 •

edited

mridulm Aug 26, 2022 •

edited

mridulm Aug 26, 2022 •

edited