siying commented on code in PR #48355:
URL: https://github.com/apache/spark/pull/48355#discussion_r1859136438
##########
common/utils/src/main/resources/error/error-conditions.json:
##########
@@ -275,7 +280,7 @@
},
"INVALID_CHANGE_LOG_READER_VERSION" : {
"message" : [
- "The change log reader version cannot be <version>."
+ "The change log reader version cannot be <version>. The checkpoint
probably is from a future Spark version, please upgrade your Spark."
Review Comment:
Perhaps soften the wording. Saying, for example, say it is possible that it
is from a future Spark version, and we shouldn't say "please upgrade your
Spark".
In case the invalid changelog comes from corruption or code bug, this error
message will be very confusing.
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/streaming/state/RocksDB.scala:
##########
@@ -278,77 +280,185 @@ class RocksDB(
// We send snapshots that needs to be uploaded by the maintenance thread to
this queue
private val snapshotsToUploadQueue = new
ConcurrentLinkedQueue[RocksDBSnapshot]()
+ /**
+ * Based on the ground truth lineage loaded from changelog file (lineage) and
+ * the latest snapshot (version, uniqueId) pair from file listing, this
function finds
+ * the ground truth latest snapshot (version, uniqueId) the db instance
needs to load.
+ *
+ * @param lineage the ground truth lineage loaded from changelog file
+ * @param latestSnapshotVersionsAndUniqueIds
+ * (version, uniqueId) pair of the latest snapshot version. In case of
task re-execution,
+ * the array could have more than one element.
+ * @return the ground truth latest snapshot (version, uniqueId) the db
instance needs to load
+ */
+ private def getLatestSnapshotVersionAndUniqueIdFromLineage(
+ lineage: Array[LineageItem],
+ latestSnapshotVersionsAndUniqueIds: Array[(Long, Option[String])]):
+ (Long, Option[String]) = {
+ lineage.foreach {
+ case LineageItem(version, uniqueId) =>
+ if (latestSnapshotVersionsAndUniqueIds.contains((version,
Some(uniqueId)))) {
+ return (version, Some(uniqueId))
+ }
+ }
+ throw
QueryExecutionErrors.cannotGetLatestSnapshotVersionAndUniqueIdFromLineage(
+ printLineageItems(lineage),
printLineage(latestSnapshotVersionsAndUniqueIds)
+ )
+ }
+
+ /**
+ * Read the lineage from the changelog files. It first get the changelog
reader
+ * of the correct changelog version and then read the lineage information
from the file.
+ * The changelog file is named as version_stateStoreCkptId.changelog
+ * @param version version of the changelog file, used to load changelog file.
+ * @param stateStoreCkptId uniqueId of the changelog file, used to load
changelog file.
+ * @return
+ */
+ private def getLineageFromChangelogFile(
+ version: Long,
+ stateStoreCkptId: Option[String]): Array[LineageItem] = {
+ var changelogReader: StateStoreChangelogReader = null
+ var currLineage: Array[LineageItem] = Array.empty
+ try {
+ changelogReader = fileManager.getChangelogReader(version,
stateStoreCkptId)
+ currLineage = changelogReader.lineage
+ logInfo(log"Loading lineage: " +
+ log"${MDC(LogKeys.LINEAGE, lineageManager)} from " +
+ log"changelog version: ${MDC(LogKeys.VERSION_NUM, version)} " +
+ log"uniqueId: ${MDC(LogKeys.UUID, stateStoreCkptId.getOrElse(""))}.")
+ } finally {
+ if (changelogReader != null) {
+ changelogReader.closeIfNeeded()
+ }
+ }
+ currLineage
+ }
+
+
/**
* Load the given version of data in a native RocksDB instance.
* Note that this will copy all the necessary file from DFS to local disk as
needed,
* and possibly restart the native RocksDB instance.
*/
- def load(
+ private def loadV2(
+ version: Long,
+ stateStoreCkptId: Option[String],
+ readOnly: Boolean = false): RocksDB = {
+ // An array contains lineage information from [snapShotVersion, version]
(inclusive both end)
+ var currVersionLineage: Array[LineageItem] = lineageManager.getLineage()
+ try {
+ if (loadedVersion != version || (loadedStateStoreCkptId.isEmpty ||
+ stateStoreCkptId.get != loadedStateStoreCkptId.get)) {
+ closeDB(ignoreException = false)
+
+ val (latestSnapshotVersion, latestSnapshotUniqueId) = {
+ // Special handling when version is 0.
+ // When loading the very first version (0), stateStoreCkptId does not
need to be defined
+ // because there won't be 0.changelog / 0.zip file created in RocksDB.
+ if (version == 0 && stateStoreCkptId.isEmpty) {
+ (0L, None)
+ // When there is a snapshot file, it is the ground truth, we can skip
+ // reconstructing the lineage from changelog file.
+ } else if (fileManager.existsSnapshotFile(version, stateStoreCkptId)) {
Review Comment:
It's OK now, but perhaps we should add a comment, saying it is not efficient
and we should check local lineage manager first.
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/streaming/state/RocksDBFileManager.scala:
##########
@@ -316,20 +346,45 @@ class RocksDBFileManager(
}
}
+ // Get latest snapshot version <= version
+ def getLatestSnapshotVersionAndUniqueId(
+ version: Long, checkpointUniqueId: Option[String] = None): Array[(Long,
Option[String])] = {
Review Comment:
Why does the return value needs to have `Option[String]` rather than
`String`? If there is no unique ID in file name, we should reject it.
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/streaming/state/RocksDB.scala:
##########
@@ -278,77 +280,185 @@ class RocksDB(
// We send snapshots that needs to be uploaded by the maintenance thread to
this queue
private val snapshotsToUploadQueue = new
ConcurrentLinkedQueue[RocksDBSnapshot]()
+ /**
+ * Based on the ground truth lineage loaded from changelog file (lineage) and
+ * the latest snapshot (version, uniqueId) pair from file listing, this
function finds
+ * the ground truth latest snapshot (version, uniqueId) the db instance
needs to load.
+ *
+ * @param lineage the ground truth lineage loaded from changelog file
+ * @param latestSnapshotVersionsAndUniqueIds
+ * (version, uniqueId) pair of the latest snapshot version. In case of
task re-execution,
+ * the array could have more than one element.
+ * @return the ground truth latest snapshot (version, uniqueId) the db
instance needs to load
+ */
+ private def getLatestSnapshotVersionAndUniqueIdFromLineage(
+ lineage: Array[LineageItem],
+ latestSnapshotVersionsAndUniqueIds: Array[(Long, Option[String])]):
+ (Long, Option[String]) = {
+ lineage.foreach {
+ case LineageItem(version, uniqueId) =>
+ if (latestSnapshotVersionsAndUniqueIds.contains((version,
Some(uniqueId)))) {
+ return (version, Some(uniqueId))
+ }
+ }
Review Comment:
nit: ChatGPT says we can simplify it to this:
```lineage.collectFirst {
case LineageItem(version, uniqueId) if
latestSnapshotVersionsAndUniqueIds.contains((version, Some(uniqueId))) =>
(version, Some(uniqueId))
}
```
I didn't try so I'm not sure whether it is the case though.
##########
common/utils/src/main/resources/error/error-conditions.json:
##########
@@ -233,6 +233,11 @@
"An error occurred during loading state."
],
"subClass" : {
+ "CANNOT_GET_LATEST_SNAPSHOT_VERSION_AND_UNIQUE_ID_FROM_LINEAGE" : {
+ "message" : [
+ "Cannot get latest snapshot version and unique ids from lineage.
lineage: <lineage>, latest snapshot versions and unique ids:
<snapshotVersionAndUniqueIds>."
Review Comment:
This is too specific too me. Perhaps just say cannot find a base snapshot
checkpoint.
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/streaming/state/RocksDBFileManager.scala:
##########
@@ -316,20 +346,45 @@ class RocksDBFileManager(
}
}
+ // Get latest snapshot version <= version
+ def getLatestSnapshotVersionAndUniqueId(
+ version: Long, checkpointUniqueId: Option[String] = None): Array[(Long,
Option[String])] = {
+ val path = new Path(dfsRootDir)
+ if (fm.exists(path)) {
+ val versionAndUniqueIds = fm.list(path, onlyZipFiles)
+ .map(_.getPath.getName.stripSuffix(".zip").split("_"))
+ .filter {
+ case Array(ver, _) => ver.toLong <= version
+ case Array(ver) => ver.toLong <= version
+ }
+ .map {
+ case Array(version, uniqueId) => (version.toLong, Option(uniqueId))
+ case Array(version) => (version.toLong, None)
Review Comment:
We should not return this one for V1.
If this function is reused in V1 and V2, we should separate it, in my
opinion. We should not return V1 and V2 files together. It is too confusing.
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/streaming/state/RocksDBFileManager.scala:
##########
@@ -316,20 +346,45 @@ class RocksDBFileManager(
}
}
+ // Get latest snapshot version <= version
+ def getLatestSnapshotVersionAndUniqueId(
+ version: Long, checkpointUniqueId: Option[String] = None): Array[(Long,
Option[String])] = {
Review Comment:
What is `checkpointUniqueId` used for here?
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/streaming/state/RocksDB.scala:
##########
@@ -278,77 +280,185 @@ class RocksDB(
// We send snapshots that needs to be uploaded by the maintenance thread to
this queue
private val snapshotsToUploadQueue = new
ConcurrentLinkedQueue[RocksDBSnapshot]()
+ /**
+ * Based on the ground truth lineage loaded from changelog file (lineage) and
+ * the latest snapshot (version, uniqueId) pair from file listing, this
function finds
+ * the ground truth latest snapshot (version, uniqueId) the db instance
needs to load.
+ *
+ * @param lineage the ground truth lineage loaded from changelog file
+ * @param latestSnapshotVersionsAndUniqueIds
+ * (version, uniqueId) pair of the latest snapshot version. In case of
task re-execution,
+ * the array could have more than one element.
+ * @return the ground truth latest snapshot (version, uniqueId) the db
instance needs to load
+ */
+ private def getLatestSnapshotVersionAndUniqueIdFromLineage(
+ lineage: Array[LineageItem],
+ latestSnapshotVersionsAndUniqueIds: Array[(Long, Option[String])]):
+ (Long, Option[String]) = {
+ lineage.foreach {
+ case LineageItem(version, uniqueId) =>
+ if (latestSnapshotVersionsAndUniqueIds.contains((version,
Some(uniqueId)))) {
+ return (version, Some(uniqueId))
+ }
+ }
+ throw
QueryExecutionErrors.cannotGetLatestSnapshotVersionAndUniqueIdFromLineage(
+ printLineageItems(lineage),
printLineage(latestSnapshotVersionsAndUniqueIds)
+ )
+ }
+
+ /**
+ * Read the lineage from the changelog files. It first get the changelog
reader
+ * of the correct changelog version and then read the lineage information
from the file.
+ * The changelog file is named as version_stateStoreCkptId.changelog
+ * @param version version of the changelog file, used to load changelog file.
+ * @param stateStoreCkptId uniqueId of the changelog file, used to load
changelog file.
+ * @return
+ */
+ private def getLineageFromChangelogFile(
+ version: Long,
+ stateStoreCkptId: Option[String]): Array[LineageItem] = {
+ var changelogReader: StateStoreChangelogReader = null
+ var currLineage: Array[LineageItem] = Array.empty
+ try {
+ changelogReader = fileManager.getChangelogReader(version,
stateStoreCkptId)
+ currLineage = changelogReader.lineage
+ logInfo(log"Loading lineage: " +
+ log"${MDC(LogKeys.LINEAGE, lineageManager)} from " +
+ log"changelog version: ${MDC(LogKeys.VERSION_NUM, version)} " +
+ log"uniqueId: ${MDC(LogKeys.UUID, stateStoreCkptId.getOrElse(""))}.")
+ } finally {
+ if (changelogReader != null) {
+ changelogReader.closeIfNeeded()
+ }
+ }
+ currLineage
+ }
+
+
/**
* Load the given version of data in a native RocksDB instance.
* Note that this will copy all the necessary file from DFS to local disk as
needed,
* and possibly restart the native RocksDB instance.
*/
- def load(
+ private def loadV2(
+ version: Long,
+ stateStoreCkptId: Option[String],
+ readOnly: Boolean = false): RocksDB = {
+ // An array contains lineage information from [snapShotVersion, version]
(inclusive both end)
+ var currVersionLineage: Array[LineageItem] = lineageManager.getLineage()
+ try {
+ if (loadedVersion != version || (loadedStateStoreCkptId.isEmpty ||
+ stateStoreCkptId.get != loadedStateStoreCkptId.get)) {
+ closeDB(ignoreException = false)
+
+ val (latestSnapshotVersion, latestSnapshotUniqueId) = {
+ // Special handling when version is 0.
+ // When loading the very first version (0), stateStoreCkptId does not
need to be defined
+ // because there won't be 0.changelog / 0.zip file created in RocksDB.
+ if (version == 0 && stateStoreCkptId.isEmpty) {
Review Comment:
Should be if version == 0, stateStoreCkptId must be empty, otherwise we
throw an exception?
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/streaming/MicroBatchExecution.scala:
##########
@@ -513,6 +513,7 @@ class MicroBatchExecution(
execCtx.startOffsets ++= execCtx.endOffsets
watermarkTracker.setWatermark(
math.max(watermarkTracker.currentWatermark,
commitMetadata.nextBatchWatermarkMs))
+ currentStateStoreCkptId ++= commitMetadata.stateUniqueIds
Review Comment:
I'm confused. If checkpoint VS not enabled, what's the ID here?
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/streaming/MicroBatchExecution.scala:
##########
@@ -965,7 +966,8 @@ class MicroBatchExecution(
updateStateStoreCkptId(execCtx, latestExecPlan)
}
execCtx.reportTimeTaken("commitOffsets") {
- if (!commitLog.add(execCtx.batchId,
CommitMetadata(watermarkTracker.currentWatermark))) {
+ if (!commitLog.add(execCtx.batchId,
+ CommitMetadata(watermarkTracker.currentWatermark,
currentStateStoreCkptId.toMap))) {
Review Comment:
If it is V1, whaat is filled here?
--
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]
