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/02/11 14:21:54 UTC
[GitHub] [spark] LuciferYang opened a new pull request #35493: [SPAPRK-38192][TESTS] Fix potential resource leak in `kvstore.RocksDBSuite`
LuciferYang opened a new pull request #35493:
URL: https://github.com/apache/spark/pull/35493
### What changes were proposed in this pull request?
`countKeys` method in `kvstore.RocksDBSuite` use `db.db().newIterator()` to create a `RocksIterator` instance, but there is no way to close it because the iterator not registered to `RocksDB.iteratorTracker`, this pr use `try-with-resources` to ensure the iterator closed.
### Why are the changes needed?
Bug fix.
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
- Pass GA
- Manual test:
**I used Apple Silicon/M1 machine to reproduce this issue**
The test steps are as follows：
1. clone a `facebook/rocksdb` code repository and switch to `6.29.fb` branch, then build a `rocksdbjni-6.29.1-osx.jar` refer to [RocksJava-Basics](https://github.com/facebook/rocksdb/wiki/RocksJava-Basics) with `WITH_LZ4` and `WITH_ZSTD` option enabled
2. change `org.rocksdb:rocksdbjni` in Spark pom.xml to use `system` scope and specify the path where `systemPath` is `/xxx/rocksdb/java/target/rocksdbjni-6.29.1-osx.jar`
3. delete `assumeFalse(SystemUtils.IS_OS_MAC_OSX && SystemUtils.OS_ARCH.equals("aarch64"));` in `RocksDBSuite` condition and ignore `testCloseRocksDBIterator` in `RocksDBSuite` because `testCloseRocksDBIterator` has another issue.
4. run mvn commands:
```
mvn clean test -pl common/kvstore -am -Dtest=org.apache.spark.util.kvstore.RocksDBSuite
```
**Before**
The VM crash as follows:
```
[ERROR] org.apache.maven.surefire.booter.SurefireBooterForkException: The forked VM terminated without properly saying goodbye. VM crash or System.exit called?
[ERROR] Command was /bin/sh -c cd /spark-source/common/kvstore && /Users/yangjie01/SourceCode/tools/zulu8u312/zulu-8.jdk/Contents/Home/jre/bin/java -ea -Xmx4g -Xss4m -XX:MaxMetaspaceSize=2g -XX:ReservedCodeCacheSize=128m -XX:+IgnoreUnrecognizedVMOptions --add-opens=java.base/java.lang=ALL-UNNAMED --add-opens=java.base/java.lang.invoke=ALL-UNNAMED --add-opens=java.base/java.lang.reflect=ALL-UNNAMED --add-opens=java.base/java.io=ALL-UNNAMED --add-opens=java.base/java.net=ALL-UNNAMED --add-opens=java.base/java.nio=ALL-UNNAMED --add-opens=java.base/java.util=ALL-UNNAMED --add-opens=java.base/java.util.concurrent=ALL-UNNAMED --add-opens=java.base/java.util.concurrent.atomic=ALL-UNNAMED --add-opens=java.base/sun.nio.ch=ALL-UNNAMED --add-opens=java.base/sun.nio.cs=ALL-UNNAMED --add-opens=java.base/sun.security.action=ALL-UNNAMED --add-opens=java.base/sun.util.calendar=ALL-UNNAMED -Dio.netty.tryReflectionSetAccessible=true -jar /spark-source/common/kvstore/target/surefire/surefirebooter48
07633958162555307.jar /spark-source/common/kvstore/target/surefire 2022-02-11T21-43-14_615-jvmRun1 surefire1545347115500361779tmp surefire_05480551952513893775tmp
[ERROR] Error occurred in starting fork, check output in log
[ERROR] Process Exit Code: 134
```
**After**
```
[INFO] Running org.apache.spark.util.kvstore.RocksDBSuite
[INFO] Tests run: 9, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 2.82 s - in org.apache.spark.util.kvstore.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] dongjoon-hyun commented on pull request #35493: [SPAPRK-38192][CORE][TESTS] Fix potential resource leak in `kvstore.RocksDBSuite`
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on pull request #35493:
URL: https://github.com/apache/spark/pull/35493#issuecomment-1036420045
Thank you, @LuciferYang !
--
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] dongjoon-hyun commented on pull request #35493: [SPAPRK-38192][CORE][TESTS] Use `try-with-resources` in `Level/RocksDBSuite.java`
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on pull request #35493:
URL: https://github.com/apache/spark/pull/35493#issuecomment-1037488781
--
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] dongjoon-hyun commented on pull request #35493: [SPARK-38192][CORE][TESTS] Use `try-with-resources` in `Level/RocksDBSuite.java`
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on pull request #35493:
URL: https://github.com/apache/spark/pull/35493#issuecomment-1038546429
Merged to master for Apache Spark 3.3.
--
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] LuciferYang commented on pull request #35493: [SPAPRK-38192][CORE][TESTS] Use `try-with-resources` in `Level/RocksDBSuite.java`
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on pull request #35493:
URL: https://github.com/apache/spark/pull/35493#issuecomment-1037682432
@dongjoon-hyun
At present, for `RocksDBSuite`, I can reproduce the issue stably, while for `LevelDBSuite`, I can only infer from the principle that it should have similar issue with `RocksDBSuite`.
Therefore, if the PR description focuses on `LevelDBSuite`, I can only speculate that it has issue in theory, and I can't provide the reproduction ways of `LevelDBSuite` for the time being.
Or should I change this pr back to the fix only for `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] dongjoon-hyun closed pull request #35493: [SPARK-38192][CORE][TESTS] Use `try-with-resources` in `Level/RocksDBSuite.java`
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun closed pull request #35493:
URL: https://github.com/apache/spark/pull/35493
--
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] LuciferYang commented on pull request #35493: [SPAPRK-38192][TESTS] Fix potential resource leak in `kvstore.RocksDBSuite`
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on pull request #35493:
URL: https://github.com/apache/spark/pull/35493#issuecomment-1036262265
cc @dongjoon-hyun
--
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] LuciferYang commented on a change in pull request #35493: [SPAPRK-38192][CORE][TESTS] Fix potential resource leak in `kvstore.RocksDBSuite`
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on a change in pull request #35493:
URL: https://github.com/apache/spark/pull/35493#discussion_r805095956
##########
File path: common/kvstore/src/test/java/org/apache/spark/util/kvstore/RocksDBSuite.java
##########
@@ -330,15 +330,16 @@ private int countKeys(Class<?> type) throws Exception {
byte[] prefix = db.getTypeInfo(type).keyPrefix();
int count = 0;
- RocksIterator it = db.db().newIterator();
- it.seek(prefix);
-
- while (it.isValid()) {
- byte[] key = it.key();
- if (RocksDBIterator.startsWith(key, prefix)) {
- count++;
+ try (RocksIterator it = db.db().newIterator()) {
Review comment:
There is also this potential risk. Let me fix it together
--
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] LuciferYang edited a comment on pull request #35493: [SPAPRK-38192][CORE][TESTS] Use `try-with-resources` in `Level/RocksDBSuite.java`
Posted by GitBox <gi...@apache.org>.
LuciferYang edited a comment on pull request #35493:
URL: https://github.com/apache/spark/pull/35493#issuecomment-1037682432
--
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] LuciferYang commented on pull request #35493: [SPAPRK-38192][CORE][TESTS] Use `try-with-resources` in `Level/RocksDBSuite.java`
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on pull request #35493:
URL: https://github.com/apache/spark/pull/35493#issuecomment-1036938739
done
--
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] LuciferYang commented on pull request #35493: [SPAPRK-38192][CORE][TESTS] Use `try-with-resources` in `Level/RocksDBSuite.java`
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on pull request #35493:
URL: https://github.com/apache/spark/pull/35493#issuecomment-1037714115
Since the pr description has been updated, I copy a reproduction step for `RocksDBSuite` here:
**I used Apple Silicon/M1 machine(MacBook Pro (13-inch, M1, 2020)) to reproduce this issue**
The test steps are as follows：
1. clone a `facebook/rocksdb` code repository and switch to `6.29.fb` branch, then build a `rocksdbjni-6.29.1-osx.jar` refer to [RocksJava-Basics](https://github.com/facebook/rocksdb/wiki/RocksJava-Basics) with `WITH_LZ4` and `WITH_ZSTD` option enabled
2. change `org.rocksdb:rocksdbjni` in Spark pom.xml to use `system` scope and specify the path where `systemPath` is `/xxx/rocksdb/java/target/rocksdbjni-6.29.1-osx.jar`
3. delete `assumeFalse(SystemUtils.IS_OS_MAC_OSX && SystemUtils.OS_ARCH.equals("aarch64"));` in `RocksDBSuite` condition and ignore `testCloseRocksDBIterator` because it has another issue.
4. run mvn commands:
```
mvn clean test -pl common/kvstore -am -Dtest=org.apache.spark.util.kvstore.RocksDBSuite
```
**Before**
The VM crash as follows:
```
[ERROR] org.apache.maven.surefire.booter.SurefireBooterForkException: The forked VM terminated without properly saying goodbye. VM crash or System.exit called?
[ERROR] Command was /bin/sh -c cd /spark-source/common/kvstore && /xxx/zulu8u312/zulu-8.jdk/Contents/Home/jre/bin/java -ea -Xmx4g -Xss4m -XX:MaxMetaspaceSize=2g -XX:ReservedCodeCacheSize=128m -XX:+IgnoreUnrecognizedVMOptions --add-opens=java.base/java.lang=ALL-UNNAMED --add-opens=java.base/java.lang.invoke=ALL-UNNAMED --add-opens=java.base/java.lang.reflect=ALL-UNNAMED --add-opens=java.base/java.io=ALL-UNNAMED --add-opens=java.base/java.net=ALL-UNNAMED --add-opens=java.base/java.nio=ALL-UNNAMED --add-opens=java.base/java.util=ALL-UNNAMED --add-opens=java.base/java.util.concurrent=ALL-UNNAMED --add-opens=java.base/java.util.concurrent.atomic=ALL-UNNAMED --add-opens=java.base/sun.nio.ch=ALL-UNNAMED --add-opens=java.base/sun.nio.cs=ALL-UNNAMED --add-opens=java.base/sun.security.action=ALL-UNNAMED --add-opens=java.base/sun.util.calendar=ALL-UNNAMED -Dio.netty.tryReflectionSetAccessible=true -jar /spark-source/common/kvstore/target/surefire/surefirebooter4807633958162555307.jar /spark-
source/common/kvstore/target/surefire 2022-02-11T21-43-14_615-jvmRun1 surefire1545347115500361779tmp surefire_05480551952513893775tmp
[ERROR] Error occurred in starting fork, check output in log
[ERROR] Process Exit Code: 134
```
**After**
```
[INFO] Running org.apache.spark.util.kvstore.RocksDBSuite
[INFO] Tests run: 9, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 2.82 s - in org.apache.spark.util.kvstore.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] dongjoon-hyun commented on pull request #35493: [SPAPRK-38192][CORE][TESTS] Use `try-with-resources` in `Level/RocksDBSuite.java`
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on pull request #35493:
URL: https://github.com/apache/spark/pull/35493#issuecomment-1037711044
Got it. Let me dig it more from my side. Thank you always.
--
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] dongjoon-hyun commented on pull request #35493: [SPAPRK-38192][CORE][TESTS] Use `try-with-resources` in `Level/RocksDBSuite.java`
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on pull request #35493:
URL: https://github.com/apache/spark/pull/35493#issuecomment-1036918282
Thank you for update. Please make the PR description up-to-date too.
--
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] LuciferYang edited a comment on pull request #35493: [SPAPRK-38192][CORE][TESTS] Use `try-with-resources` in `Level/RocksDBSuite.java`
Posted by GitBox <gi...@apache.org>.
LuciferYang edited a comment on pull request #35493:
URL: https://github.com/apache/spark/pull/35493#issuecomment-1037682432
@dongjoon-hyun Thank you for your guidance
At present, for `RocksDBSuite`, I can reproduce the issue stably, while for `LevelDBSuite`, I can only infer from the principle that it should have similar issue with `RocksDBSuite`.
Therefore, if the PR description focuses on `LevelDBSuite`, I can only speculate that it has problem in theory, and I can't provide the reproduction ways of `LevelDBSuite` for the time being.
Or should I change this pr back to the fix only for `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] LuciferYang commented on pull request #35493: [SPAPRK-38192][CORE][TESTS] Use `try-with-resources` in `Level/RocksDBSuite.java`
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on pull request #35493:
URL: https://github.com/apache/spark/pull/35493#issuecomment-1037682432
--
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] LuciferYang commented on pull request #35493: [SPAPRK-38192][CORE][TESTS] Use `try-with-resources` in `Level/RocksDBSuite.java`
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on pull request #35493:
URL: https://github.com/apache/spark/pull/35493#issuecomment-1037881018
> At the same time, in order to verify whether `LevelDB` will cause VM crash because `DBIterator ` is not closed, I try to delete some code related to resource release [here](https://github.com/apache/spark/pull/35497) and wait for the test results.
@dongjoon-hyun It seems that even if delete the registration process of `iteratorTracker` and delete the `finalize()` method in `LevelDBIterator` to not close `LevelDBIterator` at all, it will not cause VM crash on X86 platform, although it will certainly cause resource leak
--
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] LuciferYang commented on pull request #35493: [SPAPRK-38192][CORE][TESTS] Use `try-with-resources` in `Level/RocksDBSuite.java`
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on pull request #35493:
URL: https://github.com/apache/spark/pull/35493#issuecomment-1037881439
> At the same time, in order to verify whether `LevelDB` will cause VM crash because `DBIterator ` is not closed, I try to delete some code related to resource release [here](https://github.com/apache/spark/pull/35497) and wait for the test results.
@dongjoon-hyun It seems that even if delete the registration process of `iteratorTracker` and delete the `finalize()` method in `LevelDBIterator` to not close `LevelDBIterator` at all, it will not cause VM crash on X86 platform, although it will certainly cause resource leak
--
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] LuciferYang edited a comment on pull request #35493: [SPAPRK-38192][CORE][TESTS] Use `try-with-resources` in `Level/RocksDBSuite.java`
Posted by GitBox <gi...@apache.org>.
LuciferYang edited a comment on pull request #35493:
URL: https://github.com/apache/spark/pull/35493#issuecomment-1037682432
@dongjoon-hyun Thank you for your guidance
At present, for `RocksDBSuite`, I can reproduce the issue stably, while for `LevelDBSuite`, I can only infer from the principle that it should have similar issue with `RocksDBSuite`.
Therefore, if the PR description focuses on `LevelDBSuite`, I can only speculate that it has issue in theory, and I can't provide the reproduction ways of `LevelDBSuite` for the time being.
Or should I change this pr back to the fix only for `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] dongjoon-hyun commented on pull request #35493: [SPAPRK-38192][CORE][TESTS] Use `try-with-resources` in `Level/RocksDBSuite.java`
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on pull request #35493:
URL: https://github.com/apache/spark/pull/35493#issuecomment-1037488781
@LuciferYang . When you describe an issue, you had better put a bigger one first.
For example, `LevelDB*.java` was added via SPARK-20641 at Apache Spark 2.3.0 and everyone knows and is affected.
`RocksDB*.java` was added at master branch only and only active contributors (who are working on unreleased `master` branch) know.
The main different are
- If you focus on `RocksDB` mainly in the PR description, we cannot backport this patch to the older release branches.
- If you focus on `LevelDB` with the description and verification ways, we can backport this patch.
Could you refocus your PR description once more?
--
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] LuciferYang removed a comment on pull request #35493: [SPAPRK-38192][CORE][TESTS] Use `try-with-resources` in `Level/RocksDBSuite.java`
Posted by GitBox <gi...@apache.org>.
LuciferYang removed a comment on pull request #35493:
URL: https://github.com/apache/spark/pull/35493#issuecomment-1037881439
> At the same time, in order to verify whether `LevelDB` will cause VM crash because `DBIterator ` is not closed, I try to delete some code related to resource release [here](https://github.com/apache/spark/pull/35497) and wait for the test results.
@dongjoon-hyun It seems that even if delete the registration process of `iteratorTracker` and delete the `finalize()` method in `LevelDBIterator` to not close `LevelDBIterator` at all, it will not cause VM crash on X86 platform, although it will certainly cause resource leak
--
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] LuciferYang edited a comment on pull request #35493: [SPAPRK-38192][CORE][TESTS] Use `try-with-resources` in `Level/RocksDBSuite.java`
Posted by GitBox <gi...@apache.org>.
LuciferYang edited a comment on pull request #35493:
URL: https://github.com/apache/spark/pull/35493#issuecomment-1037716558
At the same time, in order to verify whether `LevelDB` will cause VM crash because `DBIterator ` is not closed, I try to delete some code related to resource release [here](https://github.com/apache/spark/pull/35497) and wait for the test results.
--
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] LuciferYang commented on pull request #35493: [SPAPRK-38192][CORE][TESTS] Use `try-with-resources` in `Level/RocksDBSuite.java`
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on pull request #35493:
URL: https://github.com/apache/spark/pull/35493#issuecomment-1037687686
@dongjoon-hyun I updated the pr description to focus on `LevelDBSuite` first , but removed the manual test reproduction way. If need to change it again, please let me know.
--
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] LuciferYang removed a comment on pull request #35493: [SPAPRK-38192][CORE][TESTS] Use `try-with-resources` in `Level/RocksDBSuite.java`
Posted by GitBox <gi...@apache.org>.
LuciferYang removed a comment on pull request #35493:
URL: https://github.com/apache/spark/pull/35493#issuecomment-1037881439
> At the same time, in order to verify whether `LevelDB` will cause VM crash because `DBIterator ` is not closed, I try to delete some code related to resource release [here](https://github.com/apache/spark/pull/35497) and wait for the test results.
@dongjoon-hyun It seems that even if delete the registration process of `iteratorTracker` and delete the `finalize()` method in `LevelDBIterator` to not close `LevelDBIterator` at all, it will not cause VM crash on X86 platform, although it will certainly cause resource leak
--
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] dongjoon-hyun commented on a change in pull request #35493: [SPAPRK-38192][CORE][TESTS] Fix potential resource leak in `kvstore.RocksDBSuite`
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #35493:
URL: https://github.com/apache/spark/pull/35493#discussion_r804843513
##########
File path: common/kvstore/src/test/java/org/apache/spark/util/kvstore/RocksDBSuite.java
##########
@@ -330,15 +330,16 @@ private int countKeys(Class<?> type) throws Exception {
byte[] prefix = db.getTypeInfo(type).keyPrefix();
int count = 0;
- RocksIterator it = db.db().newIterator();
- it.seek(prefix);
-
- while (it.isValid()) {
- byte[] key = it.key();
- if (RocksDBIterator.startsWith(key, prefix)) {
- count++;
+ try (RocksIterator it = db.db().newIterator()) {
Review comment:
What about `LevelDBSuite`?
--
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] LuciferYang edited a comment on pull request #35493: [SPAPRK-38192][CORE][TESTS] Use `try-with-resources` in `Level/RocksDBSuite.java`
Posted by GitBox <gi...@apache.org>.
LuciferYang edited a comment on pull request #35493:
URL: https://github.com/apache/spark/pull/35493#issuecomment-1037687686
@dongjoon-hyun I updated the pr description to focus on `LevelDBSuite` first , but removed the manual test reproduction way. If need to change it again or need to revert to only for `RocksDBSuite `, please let me know.
--
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] LuciferYang commented on pull request #35493: [SPARK-38192][CORE][TESTS] Use `try-with-resources` in `Level/RocksDBSuite.java`
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on pull request #35493:
URL: https://github.com/apache/spark/pull/35493#issuecomment-1038578281
thanks all
--
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] LuciferYang commented on pull request #35493: [SPAPRK-38192][CORE][TESTS] Use `try-with-resources` in `Level/RocksDBSuite.java`
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on pull request #35493:
URL: https://github.com/apache/spark/pull/35493#issuecomment-1037716558
At the same time, in order to verify whether leveldb will cause VM crash because `DBIterator ` is not closed, I try to delete some code related to resource release [here](https://github.com/apache/spark/pull/35497) and wait for the test results.
--
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] LuciferYang edited a comment on pull request #35493: [SPAPRK-38192][CORE][TESTS] Use `try-with-resources` in `Level/RocksDBSuite.java`
Posted by GitBox <gi...@apache.org>.
LuciferYang edited a comment on pull request #35493:
URL: https://github.com/apache/spark/pull/35493#issuecomment-1037881018
> At the same time, in order to verify whether `LevelDB` will cause VM crash because `DBIterator ` is not closed, I try to delete some code related to resource release [here](https://github.com/apache/spark/pull/35497) and wait for the test results.
@dongjoon-hyun It seems that even if delete the registration process of `iteratorTracker` in `LevelDB` and delete the `finalize()` method in `LevelDBIterator` to not close `LevelDBIterator` at all, it will not cause VM crash on X86 platform, although it will certainly cause resource leak
--
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] dongjoon-hyun closed pull request #35493: [SPARK-38192][CORE][TESTS] Use `try-with-resources` in `Level/RocksDBSuite.java`
Posted by GitBox <gi...@apache.org>.
dongjoon-hyun closed pull request #35493:
URL: https://github.com/apache/spark/pull/35493
--
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