-
Notifications
You must be signed in to change notification settings - Fork 921
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
[KYUUBI #5328] Batch supports priority scheduling #5352
Changes from 12 commits
654ad84
4c6b990
8d4b276
f7ca221
fcaf85d
129a467
e1705c3
67eb252
6b8d0f0
c0bbc0d
27fc5e8
21ceccb
7ed2551
1bf81e7
58621b5
687ed1e
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 |
---|---|---|
|
@@ -61,6 +61,8 @@ class JDBCMetadataStore(conf: KyuubiConf) extends MetadataStore with Logging { | |
case CUSTOM => new GenericDatabaseDialect | ||
} | ||
|
||
private val priorityEnabled = conf.get(METADATA_STORE_JDBC_PRIORITY_ENABLED) | ||
|
||
private val datasourceProperties = | ||
JDBCMetadataStoreConf.getMetadataStoreJDBCDataSourceProperties(conf) | ||
private val hikariConfig = new HikariConfig(datasourceProperties) | ||
|
@@ -167,9 +169,10 @@ class JDBCMetadataStore(conf: KyuubiConf) extends MetadataStore with Logging { | |
|request_args, | ||
|create_time, | ||
|engine_type, | ||
|cluster_manager | ||
|cluster_manager, | ||
|priority | ||
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 do not see the metadata table schema change. 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. Thanks for review, metadata table schema changed in #5329 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. thanks @zwangsheng |
||
|) | ||
|VALUES(?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) | ||
|VALUES(?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) | ||
|""".stripMargin | ||
|
||
JdbcUtils.withConnection { connection => | ||
|
@@ -190,15 +193,16 @@ class JDBCMetadataStore(conf: KyuubiConf) extends MetadataStore with Logging { | |
valueAsString(metadata.requestArgs), | ||
metadata.createTime, | ||
Option(metadata.engineType).map(_.toUpperCase(Locale.ROOT)).orNull, | ||
metadata.clusterManager.orNull) | ||
metadata.clusterManager.orNull, | ||
metadata.priority) | ||
} | ||
} | ||
|
||
override def pickMetadata(kyuubiInstance: String): Option[Metadata] = synchronized { | ||
JdbcUtils.executeQueryWithRowMapper( | ||
s"""SELECT identifier FROM $METADATA_TABLE | ||
|WHERE state=? | ||
|ORDER BY create_time ASC LIMIT 1 | ||
|ORDER BY ${if (priorityEnabled) "priority DESC," else ""} create_time ASC LIMIT 1 | ||
pan3793 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|""".stripMargin) { stmt => | ||
stmt.setString(1, OperationState.INITIALIZED.toString) | ||
} { resultSet => | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -93,4 +93,16 @@ object JDBCMetadataStoreConf { | |
.serverOnly | ||
.stringConf | ||
.createWithDefault("") | ||
|
||
val METADATA_STORE_JDBC_PRIORITY_ENABLED: ConfigEntry[Boolean] = | ||
buildConf("kyuubi.metadata.store.jdbc.priority.enabled") | ||
.doc("Whether to enable picking up jobs from the metadata store in order of priority." + | ||
"If this feature is disabled, setting kyuubi.batch.priority will not take effect." + | ||
"However, it is recommended to enable this feature if you already have a metadata store " + | ||
"that can support mixed order indexing(such as MySQL 8)." + | ||
"See why recommend this in https://github.com/apache/kyuubi/pull/5329") | ||
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. suggested change: Whether to enable the priority scheduling for batch impl v2. When false, ignore Also, please take care of the space between each line. |
||
.version("1.8.0") | ||
.serverOnly | ||
.booleanConf | ||
.createWithDefault(false) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -26,9 +26,12 @@ import org.scalatest.time.SpanSugar.convertIntToGrainOfTime | |
import org.apache.kyuubi.{KyuubiException, KyuubiFunSuite} | ||
import org.apache.kyuubi.config.KyuubiConf | ||
import org.apache.kyuubi.config.KyuubiConf._ | ||
import org.apache.kyuubi.engine.EngineType | ||
import org.apache.kyuubi.metrics.{MetricsConstants, MetricsSystem} | ||
import org.apache.kyuubi.metrics.MetricsConstants._ | ||
import org.apache.kyuubi.operation.OperationState | ||
import org.apache.kyuubi.server.metadata.api.{Metadata, MetadataFilter} | ||
import org.apache.kyuubi.server.metadata.jdbc.JDBCMetadataStoreConf.METADATA_STORE_JDBC_PRIORITY_ENABLED | ||
import org.apache.kyuubi.session.SessionType | ||
|
||
class MetadataManagerSuite extends KyuubiFunSuite { | ||
|
@@ -142,6 +145,94 @@ class MetadataManagerSuite extends KyuubiFunSuite { | |
} | ||
} | ||
|
||
test("[KYUUBI #5328] Test MetadataManager#pickBatchForSubmitting in order") { | ||
withMetadataManager(Map(METADATA_STORE_JDBC_PRIORITY_ENABLED.key -> "true")) { | ||
zwangsheng marked this conversation as resolved.
Show resolved
Hide resolved
|
||
metadataManager => | ||
val mockKyuubiInstance = "mock_kyuubi_instance" | ||
val time = System.currentTimeMillis() | ||
val mockBatchJob1 = Metadata( | ||
identifier = "mock_batch_job_1", | ||
sessionType = SessionType.BATCH, | ||
realUser = "kyuubi", | ||
username = "kyuubi", | ||
engineType = EngineType.SPARK_SQL.toString, | ||
state = OperationState.INITIALIZED.toString, | ||
priority = 20, | ||
createTime = time + 10000) | ||
val mockBatchJob2 = Metadata( | ||
identifier = "mock_batch_job_2", | ||
sessionType = SessionType.BATCH, | ||
realUser = "kyuubi", | ||
username = "kyuubi", | ||
engineType = EngineType.SPARK_SQL.toString, | ||
state = OperationState.INITIALIZED.toString, | ||
createTime = time) | ||
val mockBatchJob3 = Metadata( | ||
identifier = "mock_batch_job_3", | ||
sessionType = SessionType.BATCH, | ||
realUser = "kyuubi", | ||
username = "kyuubi", | ||
engineType = EngineType.SPARK_SQL.toString, | ||
state = OperationState.INITIALIZED.toString, | ||
createTime = time + 10000) | ||
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. Can be extracted to a dedicate method for generating mocked batch job objects? 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. Sure. Reused and modified |
||
metadataManager.insertMetadata(mockBatchJob1, asyncRetryOnError = false) | ||
metadataManager.insertMetadata(mockBatchJob2, asyncRetryOnError = false) | ||
metadataManager.insertMetadata(mockBatchJob3, asyncRetryOnError = false) | ||
|
||
// pick the highest priority batch job | ||
val metadata1 = metadataManager.pickBatchForSubmitting(mockKyuubiInstance) | ||
assert(metadata1.exists(m => m.identifier === "mock_batch_job_1")) | ||
|
||
// pick the oldest batch job when same priority | ||
val metadata2 = metadataManager.pickBatchForSubmitting(mockKyuubiInstance) | ||
assert(metadata2.exists(m => m.identifier === "mock_batch_job_2")) | ||
|
||
val metadata3 = metadataManager.pickBatchForSubmitting(mockKyuubiInstance) | ||
assert(metadata3.exists(m => m.identifier === "mock_batch_job_3")) | ||
} | ||
|
||
withMetadataManager(Map(METADATA_STORE_JDBC_PRIORITY_ENABLED.key -> "false")) { | ||
metadataManager => | ||
val mockKyuubiInstance = "mock_kyuubi_instance" | ||
val time = System.currentTimeMillis() | ||
val mockBatchJob1 = Metadata( | ||
identifier = "mock_batch_job_1", | ||
sessionType = SessionType.BATCH, | ||
realUser = "kyuubi", | ||
username = "kyuubi", | ||
engineType = EngineType.SPARK_SQL.toString, | ||
state = OperationState.INITIALIZED.toString, | ||
priority = 20, | ||
createTime = time + 10000) | ||
val mockBatchJob2 = Metadata( | ||
identifier = "mock_batch_job_2", | ||
sessionType = SessionType.BATCH, | ||
realUser = "kyuubi", | ||
username = "kyuubi", | ||
engineType = EngineType.SPARK_SQL.toString, | ||
state = OperationState.INITIALIZED.toString, | ||
createTime = time) | ||
val mockBatchJob3 = Metadata( | ||
identifier = "mock_batch_job_3", | ||
sessionType = SessionType.BATCH, | ||
realUser = "kyuubi", | ||
username = "kyuubi", | ||
engineType = EngineType.SPARK_SQL.toString, | ||
state = OperationState.INITIALIZED.toString, | ||
createTime = time + 500) | ||
metadataManager.insertMetadata(mockBatchJob1, asyncRetryOnError = false) | ||
metadataManager.insertMetadata(mockBatchJob2, asyncRetryOnError = false) | ||
metadataManager.insertMetadata(mockBatchJob3, asyncRetryOnError = false) | ||
|
||
// pick the oldest batch job | ||
val metadata2 = metadataManager.pickBatchForSubmitting(mockKyuubiInstance) | ||
assert(metadata2.exists(m => m.identifier === "mock_batch_job_2")) | ||
|
||
val metadata3 = metadataManager.pickBatchForSubmitting(mockKyuubiInstance) | ||
assert(metadata3.exists(m => m.identifier === "mock_batch_job_3")) | ||
} | ||
} | ||
|
||
private def withMetadataManager( | ||
confOverlay: Map[String, String], | ||
newMetadataMgr: () => MetadataManager = () => new MetadataManager())( | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a magic code here. Default priority is worth to be extracted to explicit static value , and further mentioned in config docs. Or it's confusing for user and developers reviewing the UTs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's at least document it should be consistent with the metadata table DDL definition