tgravescs commented on code in PR #36200: URL: https://github.com/apache/spark/pull/36200#discussion_r945826354
########## common/network-common/src/main/java/org/apache/spark/network/shuffledb/StoreVersion.java: ########## @@ -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 { Review Comment: add documentation ########## common/network-common/src/main/java/org/apache/spark/network/shuffledb/DB.java: ########## @@ -0,0 +1,47 @@ +/* + * 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; + +/** + * The local KV storage used to persist the shuffle state, + * the implementations may include leveldb, rocksdb, etc. Review Comment: nit capitalize LevelDB, RocksDB since public documentation ########## common/network-yarn/src/main/java/org/apache/spark/network/yarn/YarnShuffleService.java: ########## @@ -118,10 +121,10 @@ public class YarnShuffleService extends AuxiliaryService { private static final String SPARK_AUTHENTICATE_KEY = "spark.authenticate"; private static final boolean DEFAULT_SPARK_AUTHENTICATE = false; - private static final String RECOVERY_FILE_NAME = "registeredExecutors.ldb"; - private static final String SECRETS_RECOVERY_FILE_NAME = "sparkShuffleRecovery.ldb"; + private static final String RECOVERY_FILE_NAME = "registeredExecutors"; Review Comment: is this going to cause any compatibility issues? What about upgrading and/or changing DB? If I just upgrade to this version of spark shuffle during rolling upgrade, I assume it won't find the old file now. Seems safer to keep the .ldb when using leveldb, unless that is handled below and I haven't gotten to it yet. ########## common/network-common/src/main/java/org/apache/spark/network/shuffledb/DBBackend.java: ########## @@ -0,0 +1,42 @@ +/* + * 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.util.Locale; + +/** + * The enum `DBBackend` use to specify a disk-based store used in shuffle service local db. + * Only LEVELDB is supported now. + */ +public enum DBBackend { + LEVELDB(".ldb"); Review Comment: is the intent to make it fully pluggable or only support the versions Spark directly provides? ########## core/src/main/scala/org/apache/spark/internal/config/package.scala: ########## @@ -714,6 +715,16 @@ package object config { .booleanConf .createWithDefault(true) + private[spark] val SHUFFLE_SERVICE_DB_BACKEND = + ConfigBuilder(Constants.SHUFFLE_SERVICE_DB_BACKEND) + .doc("Specifies a disk-based store used in shuffle service local db. " + + "Only LEVELDB is supported now.") + .version("3.4.0") + .stringConf + .transform(_.toUpperCase(Locale.ROOT)) + .checkValues(DBBackend.values.map(_.toString).toSet) + .createWithDefault(DBBackend.LEVELDB.name) Review Comment: do we want this to be truly pluggable where user could just provide their own class name and we just use? If all the API's are public seems like that is the only difference. Other than perhaps documentation. I'm ok either way I guess just don't want to have to add another config or change this api if we want to go that way. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
