-
Notifications
You must be signed in to change notification settings - Fork 29.3k
[SPARK-38909][CORE][YARN] Encapsulate LevelDB used to store remote/external shuffle state as DB
#36200
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[SPARK-38909][CORE][YARN] Encapsulate LevelDB used to store remote/external shuffle state as DB
#36200
Changes from 39 commits
181a597
1e5654a
a4ae04c
29a7a1e
eacc159
ea21178
f57578e
ad5b74f
4d5739b
610e04b
c66c483
6e0a6f7
9ba181b
c80024b
1c3f09d
f03ea29
ecba36a
23753f5
4630651
712f99d
01a6da1
8e11695
9c553a8
797f4ca
d5bcce9
c6f6fa0
768f207
217df56
9b20939
515204d
28ee973
298c737
a851c88
1832b06
e147c77
004c4c9
a56f3c8
9df4fe5
7e1e562
0f7b348
c56a8f8
78d163e
bee0737
944c290
cf7658c
e3e37a7
d5a53a5
f8cb374
35bcced
2dc68c3
ee1f283
5e856f6
9590e97
b299c55
94a2831
d4ce629
9889de3
abb0a58
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,49 @@ | ||||||
| /* | ||||||
| * Licensed to the Apache Software Foundation (ASF) under one or more | ||||||
| * contributor license agreements. See the NOTICE file distributed with | ||||||
| * this work for additional information regarding copyright ownership. | ||||||
| * The ASF licenses this file to You under the Apache License, Version 2.0 | ||||||
| * (the "License"); you may not use this file except in compliance with | ||||||
| * the License. You may obtain a copy of the License at | ||||||
| * | ||||||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||||||
| * | ||||||
| * Unless required by applicable law or agreed to in writing, software | ||||||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||||||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||||||
| * See the License for the specific language governing permissions and | ||||||
| * limitations under the License. | ||||||
| */ | ||||||
|
|
||||||
| package org.apache.spark.network.shuffledb; | ||||||
|
|
||||||
| import java.io.Closeable; | ||||||
| import java.io.IOException; | ||||||
| import java.util.Map; | ||||||
|
|
||||||
| /** | ||||||
| * The local KV storage used to persist the shuffle state, | ||||||
| * the implementations may include leveldb, rocksdb, etc. | ||||||
| */ | ||||||
| public interface DB extends Closeable { | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this a developer API or user API? I didn't see the custom DB could be plugged in this PR.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. developer API. The implementation of RocksDB is under review: #37610,
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So could you add annotation
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We do not support users plugging in a different DB instance - it comes from within spark code.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mridulm It's a public interface. I think it should either be a user API or developer API, no?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hmm... should we use spark/common/kvstore/src/main/java/org/apache/spark/util/kvstore/KVStore.java Lines 65 to 66 in 52cd037
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The configuration is public @Ngone51, not the implementation itself. I am fine with marking it as It is the same for most submodules in common/* - except probably for common/kvstore, where Marcello marked the interfaces as @Private :-)
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mridulm @LuciferYang Thanks. |
||||||
| /** | ||||||
| * Set the DB entry for "key" to "value". | ||||||
| */ | ||||||
| void put(byte[] key, byte[] value) throws RuntimeException; | ||||||
|
|
||||||
| /** | ||||||
| * Get which returns a new byte array storing the value associated | ||||||
| * with the specified input key if any. | ||||||
| */ | ||||||
| byte[] get(byte[] key) throws RuntimeException; | ||||||
|
|
||||||
| /** | ||||||
| * Delete the DB entry (if any) for "key". | ||||||
| */ | ||||||
| void delete(byte[] key) throws RuntimeException; | ||||||
|
|
||||||
| /** | ||||||
| * Read KV prefixed with `prefix` into a Map from DB. | ||||||
| */ | ||||||
| Map<String, byte[]> readKVToMap(String prefix) throws IOException; | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Discussion:
Given both, do we want to make this an
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Refactoring
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 0f7b348 Introduce |
||||||
| } | ||||||
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
| @@ -0,0 +1,69 @@ | ||||
| /* | ||||
| * Licensed to the Apache Software Foundation (ASF) under one or more | ||||
| * contributor license agreements. See the NOTICE file distributed with | ||||
| * this work for additional information regarding copyright ownership. | ||||
| * The ASF licenses this file to You under the Apache License, Version 2.0 | ||||
| * (the "License"); you may not use this file except in compliance with | ||||
| * the License. You may obtain a copy of the License at | ||||
| * | ||||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||||
| * | ||||
| * Unless required by applicable law or agreed to in writing, software | ||||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||||
| * See the License for the specific language governing permissions and | ||||
| * limitations under the License. | ||||
| */ | ||||
|
|
||||
| package org.apache.spark.network.shuffledb; | ||||
|
|
||||
| import org.iq80.leveldb.DBIterator; | ||||
|
|
||||
| import java.io.IOException; | ||||
| import java.nio.charset.StandardCharsets; | ||||
| import java.util.HashMap; | ||||
| import java.util.Map; | ||||
|
|
||||
| public class LevelDB implements DB { | ||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm wondering if we can reuse the following.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 to the idea of leveraging existing code in kvstore. Right now, this part of the code is only being used by HistoryServer. But I think it is reasonable to either extend or refactor to make it being used for DBs in Shuffle.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. a little difficult, as @mridulm said:
|
||||
| private final org.iq80.leveldb.DB db; | ||||
|
|
||||
| public LevelDB(org.iq80.leveldb.DB db) { | ||||
| this.db = db; | ||||
| } | ||||
|
|
||||
| @Override | ||||
| public void put(byte[] key, byte[] value) throws RuntimeException { | ||||
| db.put(key, value); | ||||
| } | ||||
|
|
||||
| @Override | ||||
| public byte[] get(byte[] key) throws RuntimeException { | ||||
|
LuciferYang marked this conversation as resolved.
Outdated
|
||||
| return db.get(key); | ||||
| } | ||||
|
|
||||
| @Override | ||||
| public void delete(byte[] key) throws RuntimeException { | ||||
| db.delete(key); | ||||
| } | ||||
|
|
||||
| @Override | ||||
| public void close() throws IOException { | ||||
| db.close(); | ||||
| } | ||||
|
|
||||
| @Override | ||||
| public Map<String, byte[]> readKVToMap(String prefix) { | ||||
| Map<String, byte[]> map = new HashMap<>(); | ||||
| DBIterator itr = db.iterator(); | ||||
| itr.seek(prefix.getBytes(StandardCharsets.UTF_8)); | ||||
| while (itr.hasNext()) { | ||||
| Map.Entry<byte[], byte[]> e = itr.next(); | ||||
| String key = new String(e.getKey(), StandardCharsets.UTF_8); | ||||
| if (!key.startsWith(prefix)) { | ||||
| break; | ||||
| } | ||||
| map.put(key, e.getValue()); | ||||
| } | ||||
| return map; | ||||
| } | ||||
| } | ||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,54 @@ | ||
| /* | ||
| * Licensed to the Apache Software Foundation (ASF) under one or more | ||
| * contributor license agreements. See the NOTICE file distributed with | ||
| * this work for additional information regarding copyright ownership. | ||
| * The ASF licenses this file to You under the Apache License, Version 2.0 | ||
| * (the "License"); you may not use this file except in compliance with | ||
| * the License. You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
|
|
||
| package org.apache.spark.network.shuffledb; | ||
|
|
||
| import java.nio.charset.StandardCharsets; | ||
|
|
||
| import com.fasterxml.jackson.annotation.JsonCreator; | ||
| import com.fasterxml.jackson.annotation.JsonProperty; | ||
|
|
||
| public class StoreVersion { | ||
|
tgravescs marked this conversation as resolved.
|
||
|
|
||
| public static final byte[] KEY = "StoreVersion".getBytes(StandardCharsets.UTF_8); | ||
|
|
||
| public final int major; | ||
| public final int minor; | ||
|
|
||
| @JsonCreator | ||
| public StoreVersion(@JsonProperty("major") int major, @JsonProperty("minor") int minor) { | ||
| this.major = major; | ||
| this.minor = minor; | ||
| } | ||
|
|
||
| @Override | ||
| public boolean equals(Object o) { | ||
| if (this == o) return true; | ||
| if (o == null || getClass() != o.getClass()) return false; | ||
|
|
||
| StoreVersion that = (StoreVersion) o; | ||
|
|
||
| return major == that.major && minor == that.minor; | ||
| } | ||
|
|
||
| @Override | ||
| public int hashCode() { | ||
| int result = major; | ||
| result = 31 * result + minor; | ||
| return result; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,51 @@ | ||
| /* | ||
| * Licensed to the Apache Software Foundation (ASF) under one or more | ||
| * contributor license agreements. See the NOTICE file distributed with | ||
| * this work for additional information regarding copyright ownership. | ||
| * The ASF licenses this file to You under the Apache License, Version 2.0 | ||
| * (the "License"); you may not use this file except in compliance with | ||
| * the License. You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
| package org.apache.spark.network.util; | ||
|
|
||
| import java.io.File; | ||
| import java.io.IOException; | ||
|
|
||
| import com.fasterxml.jackson.databind.ObjectMapper; | ||
| import com.google.common.annotations.VisibleForTesting; | ||
|
|
||
| import org.apache.spark.network.shuffledb.LevelDB; | ||
| import org.apache.spark.network.shuffledb.DB; | ||
| import org.apache.spark.network.shuffledb.StoreVersion; | ||
|
|
||
| public class DBProvider { | ||
| public static DB initDB(File dbFile, StoreVersion version, ObjectMapper mapper) | ||
| throws IOException { | ||
| if (dbFile != null) { | ||
| if (dbFile.getName().endsWith(".ldb")) { | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use file suffix to determine the DB type now, I haven't find better way |
||
| org.iq80.leveldb.DB levelDB = LevelDBProvider.initLevelDB(dbFile, version, mapper); | ||
| return levelDB != null ? new LevelDB(levelDB) : null; | ||
| } else { | ||
| return null; | ||
| } | ||
| } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of relying on extension, we should make it explicit - for example what is in SPARK-37680.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let me think about it
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. cc314e6 add a new config
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Currently, the type of If re-parse the type of Do you have any suggestions for this? @mridulm
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 35bcced change to pass |
||
| return null; | ||
| } | ||
|
|
||
| @VisibleForTesting | ||
| public static DB initDB(File file) throws IOException { | ||
| if (file.getName().endsWith(".ldb")) { | ||
| return new LevelDB(LevelDBProvider.initLevelDB(file)); | ||
| } else { | ||
| return null; | ||
| } | ||
| } | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.