You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2022/04/18 09:06:48 UTC
[GitHub] [spark] Myasuka opened a new pull request, #36242: [SPARK-38931][SS] Create root dfs directory for RocksDBFileManager with unknown number of keys on 1st checkpoint
Myasuka opened a new pull request, #36242:
URL: https://github.com/apache/spark/pull/36242
### What changes were proposed in this pull request?
Create root dfs directory for RocksDBFileManager with unknown number of keys on 1st checkpoint.
### Why are the changes needed?
If this fix is not introduced, we might meet exception below:
~~~java
File /private/var/folders/rk/wyr101_562ngn8lp7tbqt7_00000gp/T/spark-ce4a0607-b1d8-43b8-becd-638c6b030019/state/1/1 does not exist
java.io.FileNotFoundException: File /private/var/folders/rk/wyr101_562ngn8lp7tbqt7_00000gp/T/spark-ce4a0607-b1d8-43b8-becd-638c6b030019/state/1/1 does not exist
at org.apache.hadoop.fs.RawLocalFileSystem.deprecatedGetFileStatus(RawLocalFileSystem.java:779)
at org.apache.hadoop.fs.RawLocalFileSystem.getFileLinkStatusInternal(RawLocalFileSystem.java:1100)
at org.apache.hadoop.fs.RawLocalFileSystem.getFileStatus(RawLocalFileSystem.java:769)
at org.apache.hadoop.fs.DelegateToFileSystem.getFileStatus(DelegateToFileSystem.java:128)
at org.apache.hadoop.fs.DelegateToFileSystem.createInternal(DelegateToFileSystem.java:93)
at org.apache.hadoop.fs.ChecksumFs$ChecksumFSOutputSummer.<init>(ChecksumFs.java:353)
at org.apache.hadoop.fs.ChecksumFs.createInternal(ChecksumFs.java:400)
at org.apache.hadoop.fs.AbstractFileSystem.create(AbstractFileSystem.java:626)
at org.apache.hadoop.fs.FileContext$3.next(FileContext.java:701)
at org.apache.hadoop.fs.FileContext$3.next(FileContext.java:697)
at org.apache.hadoop.fs.FSLinkResolver.resolve(FSLinkResolver.java:90)
at org.apache.hadoop.fs.FileContext.create(FileContext.java:703)
at org.apache.spark.sql.execution.streaming.FileContextBasedCheckpointFileManager.createTempFile(CheckpointFileManager.scala:327)
at org.apache.spark.sql.execution.streaming.CheckpointFileManager$RenameBasedFSDataOutputStream.<init>(CheckpointFileManager.scala:140)
at org.apache.spark.sql.execution.streaming.CheckpointFileManager$RenameBasedFSDataOutputStream.<init>(CheckpointFileManager.scala:143)
at org.apache.spark.sql.execution.streaming.FileContextBasedCheckpointFileManager.createAtomic(CheckpointFileManager.scala:333)
at org.apache.spark.sql.execution.streaming.state.RocksDBFileManager.zipToDfsFile(RocksDBFileManager.scala:438)
at org.apache.spark.sql.execution.streaming.state.RocksDBFileManager.saveCheckpointToDfs(RocksDBFileManager.scala:174)
at org.apache.spark.sql.execution.streaming.state.RocksDBSuite.saveCheckpointFiles(RocksDBSuite.scala:566)
at org.apache.spark.sql.execution.streaming.state.RocksDBSuite.$anonfun$new$35(RocksDBSuite.scala:179)
........
~~~
### Does this PR introduce _any_ user-facing change?
No.
### How was this patch tested?
Tested via RocksDBSuite.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HeartSaVioR commented on a diff in pull request #36242: [SPARK-38931][SS] Create root dfs directory for RocksDBFileManager with unknown number of keys on 1st checkpoint
Posted by GitBox <gi...@apache.org>.
HeartSaVioR commented on code in PR #36242:
URL: https://github.com/apache/spark/pull/36242#discussion_r852068799
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/streaming/state/RocksDBSuite.scala:
##########
@@ -168,6 +168,22 @@ class RocksDBSuite extends SparkFunSuite {
}
}
+ test("RocksDBFileManager: create init dfs directory with unknown number of keys") {
+ val dfsRootDir = new File(Utils.createTempDir().getAbsolutePath + "/state/1/1")
Review Comment:
Ah OK. Good point.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HeartSaVioR commented on pull request #36242: [SPARK-38931][SS] Create root dfs directory for RocksDBFileManager with unknown number of keys on 1st checkpoint
Posted by GitBox <gi...@apache.org>.
HeartSaVioR commented on PR #36242:
URL: https://github.com/apache/spark/pull/36242#issuecomment-1102556185
ASF JIRA seems to be down. I'll update the JIRA manually tomorrow.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HeartSaVioR commented on pull request #36242: [SPARK-38931][SS] Create root dfs directory for RocksDBFileManager with unknown number of keys on 1st checkpoint
Posted by GitBox <gi...@apache.org>.
HeartSaVioR commented on PR #36242:
URL: https://github.com/apache/spark/pull/36242#issuecomment-1101328587
GA: https://github.com/Myasuka/spark/actions/runs/2183195609
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] Myasuka commented on a diff in pull request #36242: [SPARK-38931][SS] Create root dfs directory for RocksDBFileManager with unknown number of keys on 1st checkpoint
Posted by GitBox <gi...@apache.org>.
Myasuka commented on code in PR #36242:
URL: https://github.com/apache/spark/pull/36242#discussion_r852073120
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/streaming/state/RocksDBSuite.scala:
##########
@@ -168,6 +168,22 @@ class RocksDBSuite extends SparkFunSuite {
}
}
+ test("RocksDBFileManager: create init dfs directory with unknown number of keys") {
+ val dfsRootDir = new File(Utils.createTempDir().getAbsolutePath + "/state/1/1")
+ try {
+ val fileManager = new RocksDBFileManager(
+ dfsRootDir.getAbsolutePath, Utils.createTempDir(), new Configuration)
+ // Save a version of empty checkpoint files
+ val cpFiles = Seq()
+ assert(!dfsRootDir.exists())
+ saveCheckpointFiles(fileManager, cpFiles, version = 1, numKeys = -1)
Review Comment:
Why we need to load the checkpoint here? The failure happens during snapshot.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HeartSaVioR commented on pull request #36242: [SPARK-38931][SS] Create root dfs directory for RocksDBFileManager with unknown number of keys on 1st checkpoint
Posted by GitBox <gi...@apache.org>.
HeartSaVioR commented on PR #36242:
URL: https://github.com/apache/spark/pull/36242#issuecomment-1104683768
Yeah thanks for the info! Just wanted to scope the impact of the bug and realized it hasn't been released. :) And happy to hear this significantly helps in some use case.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HeartSaVioR commented on a diff in pull request #36242: [SPARK-38931][SS] Create root dfs directory for RocksDBFileManager with unknown number of keys on 1st checkpoint
Posted by GitBox <gi...@apache.org>.
HeartSaVioR commented on code in PR #36242:
URL: https://github.com/apache/spark/pull/36242#discussion_r852056800
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/streaming/state/RocksDBSuite.scala:
##########
@@ -168,6 +168,22 @@ class RocksDBSuite extends SparkFunSuite {
}
}
+ test("RocksDBFileManager: create init dfs directory with unknown number of keys") {
+ val dfsRootDir = new File(Utils.createTempDir().getAbsolutePath + "/state/1/1")
Review Comment:
Let's simply use `withTempDir` like below test.
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/streaming/state/RocksDBSuite.scala:
##########
@@ -168,6 +168,22 @@ class RocksDBSuite extends SparkFunSuite {
}
}
+ test("RocksDBFileManager: create init dfs directory with unknown number of keys") {
+ val dfsRootDir = new File(Utils.createTempDir().getAbsolutePath + "/state/1/1")
+ try {
+ val fileManager = new RocksDBFileManager(
+ dfsRootDir.getAbsolutePath, Utils.createTempDir(), new Configuration)
+ // Save a version of empty checkpoint files
+ val cpFiles = Seq()
+ assert(!dfsRootDir.exists())
+ saveCheckpointFiles(fileManager, cpFiles, version = 1, numKeys = -1)
Review Comment:
Could we verify the result with `loadAndVerifyCheckpointFiles(fileManager, verificationDir, version = 1, Nil, -1)`?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] Myasuka commented on a diff in pull request #36242: [SPARK-38931][SS] Create root dfs directory for RocksDBFileManager with unknown number of keys on 1st checkpoint
Posted by GitBox <gi...@apache.org>.
Myasuka commented on code in PR #36242:
URL: https://github.com/apache/spark/pull/36242#discussion_r852066986
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/streaming/state/RocksDBSuite.scala:
##########
@@ -168,6 +168,22 @@ class RocksDBSuite extends SparkFunSuite {
}
}
+ test("RocksDBFileManager: create init dfs directory with unknown number of keys") {
+ val dfsRootDir = new File(Utils.createTempDir().getAbsolutePath + "/state/1/1")
Review Comment:
`withTempDir` would create the directory, however, I don't want to create the directory. Otherwise, it would not report the file not found exception.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HeartSaVioR commented on pull request #36242: [SPARK-38931][SS] Create root dfs directory for RocksDBFileManager with unknown number of keys on 1st checkpoint
Posted by GitBox <gi...@apache.org>.
HeartSaVioR commented on PR #36242:
URL: https://github.com/apache/spark/pull/36242#issuecomment-1104672360
@Myasuka
Hi, I just indicated SPARK-37224 is only landed to Spark 3.3.0 which is not released yet. (I was confused since affected version is set to 3.2.1 in the JIRA ticket.) Have you been using the custom version of Spark?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HeartSaVioR commented on a diff in pull request #36242: [SPARK-38931][SS] Create root dfs directory for RocksDBFileManager with unknown number of keys on 1st checkpoint
Posted by GitBox <gi...@apache.org>.
HeartSaVioR commented on code in PR #36242:
URL: https://github.com/apache/spark/pull/36242#discussion_r852505881
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/streaming/state/RocksDBSuite.scala:
##########
@@ -168,6 +168,22 @@ class RocksDBSuite extends SparkFunSuite {
}
}
+ test("RocksDBFileManager: create init dfs directory with unknown number of keys") {
+ val dfsRootDir = new File(Utils.createTempDir().getAbsolutePath + "/state/1/1")
+ try {
+ val fileManager = new RocksDBFileManager(
+ dfsRootDir.getAbsolutePath, Utils.createTempDir(), new Configuration)
+ // Save a version of empty checkpoint files
+ val cpFiles = Seq()
+ assert(!dfsRootDir.exists())
+ saveCheckpointFiles(fileManager, cpFiles, version = 1, numKeys = -1)
Review Comment:
In majority of cases we do symmetric test - if you save checkpoint for verification, you are encouraged to load the checkpoint just you saved to verify it completely.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] Myasuka commented on pull request #36242: [SPARK-38931][SS] Create root dfs directory for RocksDBFileManager with unknown number of keys on 1st checkpoint
Posted by GitBox <gi...@apache.org>.
Myasuka commented on PR #36242:
URL: https://github.com/apache/spark/pull/36242#issuecomment-1104680419
@HeartSaVioR , we just tested the performance of RocksDB state store in Spark structured streaming, and noticed that the performance impact of trackTotalNumberOfRows is really significant in some cases. To be fair, I pick the feature of disabling the tracking number of rows, unfortunately, it then hit the bug of SPARK-38931 mentioned.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HeartSaVioR commented on a diff in pull request #36242: [SPARK-38931][SS] Create root dfs directory for RocksDBFileManager with unknown number of keys on 1st checkpoint
Posted by GitBox <gi...@apache.org>.
HeartSaVioR commented on code in PR #36242:
URL: https://github.com/apache/spark/pull/36242#discussion_r852505881
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/streaming/state/RocksDBSuite.scala:
##########
@@ -168,6 +168,22 @@ class RocksDBSuite extends SparkFunSuite {
}
}
+ test("RocksDBFileManager: create init dfs directory with unknown number of keys") {
+ val dfsRootDir = new File(Utils.createTempDir().getAbsolutePath + "/state/1/1")
+ try {
+ val fileManager = new RocksDBFileManager(
+ dfsRootDir.getAbsolutePath, Utils.createTempDir(), new Configuration)
+ // Save a version of empty checkpoint files
+ val cpFiles = Seq()
+ assert(!dfsRootDir.exists())
+ saveCheckpointFiles(fileManager, cpFiles, version = 1, numKeys = -1)
Review Comment:
In majority of cases we do symmetric test - if you save checkpoint for verification, you are encouraged to load the checkpoint just you saved to verify it completely.
This is already something like acceptance test, not pure unit test. You can add additional check to verify the part of your fix and say that could cover your fix like pure unit test, but then you really want to do pure unit test, not from RocksDBSuite but from RocksDBFileManagerSuite, specifically testing `fileManager.saveCheckpointToDfs`.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HeartSaVioR commented on a diff in pull request #36242: [SPARK-38931][SS] Create root dfs directory for RocksDBFileManager with unknown number of keys on 1st checkpoint
Posted by GitBox <gi...@apache.org>.
HeartSaVioR commented on code in PR #36242:
URL: https://github.com/apache/spark/pull/36242#discussion_r852505881
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/streaming/state/RocksDBSuite.scala:
##########
@@ -168,6 +168,22 @@ class RocksDBSuite extends SparkFunSuite {
}
}
+ test("RocksDBFileManager: create init dfs directory with unknown number of keys") {
+ val dfsRootDir = new File(Utils.createTempDir().getAbsolutePath + "/state/1/1")
+ try {
+ val fileManager = new RocksDBFileManager(
+ dfsRootDir.getAbsolutePath, Utils.createTempDir(), new Configuration)
+ // Save a version of empty checkpoint files
+ val cpFiles = Seq()
+ assert(!dfsRootDir.exists())
+ saveCheckpointFiles(fileManager, cpFiles, version = 1, numKeys = -1)
Review Comment:
In majority of cases we do symmetric test - if you save checkpoint for verification, you are encouraged to load the checkpoint just you saved to verify it completely.
This is already something like acceptance test, not pure unit test. You can do the specific check to verify the part of your fix and say that could cover your fix like pure unit test, but then you really want to do pure unit test, not from RocksDBSuite but from RocksDBFileManagerSuite, specifically testing `fileManager.saveCheckpointToDfs`.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] Myasuka commented on a diff in pull request #36242: [SPARK-38931][SS] Create root dfs directory for RocksDBFileManager with unknown number of keys on 1st checkpoint
Posted by GitBox <gi...@apache.org>.
Myasuka commented on code in PR #36242:
URL: https://github.com/apache/spark/pull/36242#discussion_r852568415
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/streaming/state/RocksDBSuite.scala:
##########
@@ -168,6 +168,22 @@ class RocksDBSuite extends SparkFunSuite {
}
}
+ test("RocksDBFileManager: create init dfs directory with unknown number of keys") {
+ val dfsRootDir = new File(Utils.createTempDir().getAbsolutePath + "/state/1/1")
+ try {
+ val fileManager = new RocksDBFileManager(
+ dfsRootDir.getAbsolutePath, Utils.createTempDir(), new Configuration)
+ // Save a version of empty checkpoint files
+ val cpFiles = Seq()
+ assert(!dfsRootDir.exists())
+ saveCheckpointFiles(fileManager, cpFiles, version = 1, numKeys = -1)
Review Comment:
Previously, I prefer pure unit test. And I see your points, already updated in the force pushed branch.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] AmplabJenkins commented on pull request #36242: [SPARK-38931][SS] Create root dfs directory for RocksDBFileManager with unknown number of keys on 1st checkpoint
Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on PR #36242:
URL: https://github.com/apache/spark/pull/36242#issuecomment-1101788651
Can one of the admins verify this patch?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HeartSaVioR closed pull request #36242: [SPARK-38931][SS] Create root dfs directory for RocksDBFileManager with unknown number of keys on 1st checkpoint
Posted by GitBox <gi...@apache.org>.
HeartSaVioR closed pull request #36242: [SPARK-38931][SS] Create root dfs directory for RocksDBFileManager with unknown number of keys on 1st checkpoint
URL: https://github.com/apache/spark/pull/36242
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HeartSaVioR commented on pull request #36242: [SPARK-38931][SS] Create root dfs directory for RocksDBFileManager with unknown number of keys on 1st checkpoint
Posted by GitBox <gi...@apache.org>.
HeartSaVioR commented on PR #36242:
URL: https://github.com/apache/spark/pull/36242#issuecomment-1102379048
Thanks! Merging to master/3.3/3.2 (I'll do this around an hour later though.)
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HeartSaVioR commented on a diff in pull request #36242: [SPARK-38931][SS] Create root dfs directory for RocksDBFileManager with unknown number of keys on 1st checkpoint
Posted by GitBox <gi...@apache.org>.
HeartSaVioR commented on code in PR #36242:
URL: https://github.com/apache/spark/pull/36242#discussion_r852505881
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/streaming/state/RocksDBSuite.scala:
##########
@@ -168,6 +168,22 @@ class RocksDBSuite extends SparkFunSuite {
}
}
+ test("RocksDBFileManager: create init dfs directory with unknown number of keys") {
+ val dfsRootDir = new File(Utils.createTempDir().getAbsolutePath + "/state/1/1")
+ try {
+ val fileManager = new RocksDBFileManager(
+ dfsRootDir.getAbsolutePath, Utils.createTempDir(), new Configuration)
+ // Save a version of empty checkpoint files
+ val cpFiles = Seq()
+ assert(!dfsRootDir.exists())
+ saveCheckpointFiles(fileManager, cpFiles, version = 1, numKeys = -1)
Review Comment:
In majority of cases we do symmetric test - if you save checkpoint for verification, you are encouraged to load the checkpoint just you saved to verify it completely.
This is already something like acceptance test, not pure unit test. You can do the specific check to verify the part of your fix and say the test could cover your fix (like pure unit test), but then you really want to do pure unit test, not from RocksDBSuite but from RocksDBFileManagerSuite, specifically testing `fileManager.saveCheckpointToDfs`.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HeartSaVioR commented on pull request #36242: [SPARK-38931][SS] Create root dfs directory for RocksDBFileManager with unknown number of keys on 1st checkpoint
Posted by GitBox <gi...@apache.org>.
HeartSaVioR commented on PR #36242:
URL: https://github.com/apache/spark/pull/36242#issuecomment-1101887727
OK thanks for confirming. I just wanted to see whether the config helps on the production workload or not - in microbenchmark we verified it definitely helps, but knowing the real impact on production workload is even much better.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] Myasuka commented on pull request #36242: [SPARK-38931][SS] Create root dfs directory for RocksDBFileManager with unknown number of keys on 1st checkpoint
Posted by GitBox <gi...@apache.org>.
Myasuka commented on PR #36242:
URL: https://github.com/apache/spark/pull/36242#issuecomment-1101351423
@HeartSaVioR I had manual verified the behavior with my patch before, and it works.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] HeartSaVioR commented on pull request #36242: [SPARK-38931][SS] Create root dfs directory for RocksDBFileManager with unknown number of keys on 1st checkpoint
Posted by GitBox <gi...@apache.org>.
HeartSaVioR commented on PR #36242:
URL: https://github.com/apache/spark/pull/36242#issuecomment-1102368202
GA passed for the commit https://github.com/apache/spark/commit/302ff2aa7812ef64dfdf6e7ce1f5eb008e66db7d - https://github.com/Myasuka/spark/actions/runs/2187377725
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org