[SPARK-41524][SS] Differentiate SQLConf and extraOptions in StateStoreConf for its usage in RocksDBConf #39069
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…eConf for its usage in RocksDBConf
HeartSaVioR
reviewed
Dec 15, 2022
|
|
||
| private case class ConfEntry(name: String, default: String) { | ||
| def fullName: String = s"$ROCKSDB_CONF_NAME_PREFIX.${name}".toLowerCase(Locale.ROOT) | ||
| def sqlConfFullName: String = s"$ROCKSDB_SQL_CONF_NAME_PREFIX.${name}".toLowerCase(Locale.ROOT) | ||
| def lowerCaseName: String = name.toLowerCase(Locale.ROOT) |
There was a problem hiding this comment.
What about having different class for each case instead? It would be clearer to distinguish configs, e.g. we don't expect the same config to be set from both SQL conf and extraOptions. If we allow such case, it'll be a bit more complicated than what you proposed, e.g. we will have to define preference.
There was a problem hiding this comment.
After the change, the method name does not need additional FromSQL or FromExtraOptions since parameter will tell the fact.
HeartSaVioR
reviewed
Dec 15, 2022
sql/core/src/main/scala/org/apache/spark/sql/execution/streaming/state/StateStoreConf.scala
Outdated
Show resolved
Hide resolved
HeartSaVioR
changed the title
HeartSaVioR
reviewed
Dec 15, 2022
| @@ -560,21 +560,35 @@ case class RocksDBConf( | |||
|
|
|||
| object RocksDBConf { | |||
| /** Common prefix of all confs in SQLConf that affects RocksDB */ | |||
| val ROCKSDB_CONF_NAME_PREFIX = "spark.sql.streaming.stateStore.rocksdb" | |||
| val ROCKSDB_SQL_CONF_NAME_PREFIX = "spark.sql.streaming.stateStore.rocksdb" | |||
|
|
|||
| private case class ConfEntry(name: String, default: String) { | |||
There was a problem hiding this comment.
abstract class or trait. extending case class is a bad practice for Scala.
EDIT: It's not just a bad practice. It's disallowed.
|
Thanks! Merging to master. |
|
Can one of the admins verify this patch? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
Currently the usage of StateStoreConf is via confs, which composes both SQL confs and extraOptions into one. The name of config for extraOptions shouldn't have to follow the name prefix of SQL conf, because it's not bound to the context of SQL conf.
Why are the changes needed?
After differentiate SQL conf and extraOptions in StateStoreConf, we should be able to adopt more use case on operator level configs by using the extraOptions.
Does this PR introduce any user-facing change?
No
How was this patch tested?
Existing UT should cover the change