You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@uniffle.apache.org by GitBox <gi...@apache.org> on 2022/09/06 10:32:23 UTC
[GitHub] [incubator-uniffle] LuciferYang opened a new pull request, #203: [ISSUE-144] Simplify implementation of `destroyDirectByteBuffer` method and enhance the stability of related test
LuciferYang opened a new pull request, #203:
URL: https://github.com/apache/incubator-uniffle/pull/203
### What changes were proposed in this pull request?
The main change of this pr as follows:
- Simplify implementation of `RssShuffleUtils#destroyDirectByteBuffer` method: the original implementation uses reflection, and this pr change to call `DirectBuffer.cleaner().clean()` directly.
- enhance the stability of `RssShuffleUtilsTest#testDestroyDirectByteBuffer`: change to use address validity to confirm that memory is released
### Why are the changes needed?
Simplify code and make testing more stability.
### Does this PR introduce _any_ user-facing change?
No
### How was this patch tested?
- Existing tests
- Manual test
run the following command on MacOS/Apple Silicon environment:
```
mvn clean install -pl common -am -DskipTests
mvn test -pl common -Dtest=org.apache.uniffle.common.RssShuffleUtilsTest#testDestroyDirectByteBuffer
```
**Before**
```
[ERROR] Tests run: 1, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 0.302 s <<< FAILURE! - in org.apache.uniffle.common.RssShuffleUtilsTest
[ERROR] testDestroyDirectByteBuffer Time elapsed: 0.3 s <<< FAILURE!
org.opentest4j.AssertionFailedError: expected: <true> but was: <false>
at org.junit.jupiter.api.AssertionUtils.fail(AssertionUtils.java:55)
at org.junit.jupiter.api.AssertTrue.assertTrue(AssertTrue.java:40)
at org.junit.jupiter.api.AssertTrue.assertTrue(AssertTrue.java:35)
at org.junit.jupiter.api.Assertions.assertTrue(Assertions.java:179)
at org.apache.uniffle.common.RssShuffleUtilsTest.testDestroyDirectByteBuffer(RssShuffleUtilsTest.java:72)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.lang.reflect.Method.invoke(Method.java:498)
at org.junit.platform.commons.util.ReflectionUtils.invokeMethod(ReflectionUtils.java:725)
at org.junit.jupiter.engine.execution.MethodInvocation.proceed(MethodInvocation.java:60)
at org.junit.jupiter.engine.execution.InvocationInterceptorChain$ValidatingInvocation.proceed(InvocationInterceptorChain.java:131)
at org.junit.jupiter.engine.extension.TimeoutExtension.intercept(TimeoutExtension.java:149)
at org.junit.jupiter.engine.extension.TimeoutExtension.interceptTestableMethod(TimeoutExtension.java:140)
at org.junit.jupiter.engine.extension.TimeoutExtension.interceptTestMethod(TimeoutExtension.java:84)
at org.junit.jupiter.engine.execution.ExecutableInvoker$ReflectiveInterceptorCall.lambda$ofVoidMethod$0(ExecutableInvoker.java:115)
at org.junit.jupiter.engine.execution.ExecutableInvoker.lambda$invoke$0(ExecutableInvoker.java:105)
at org.junit.jupiter.engine.execution.InvocationInterceptorChain$InterceptedInvocation.proceed(InvocationInterceptorChain.java:106)
at org.junit.jupiter.engine.execution.InvocationInterceptorChain.proceed(InvocationInterceptorChain.java:64)
at org.junit.jupiter.engine.execution.InvocationInterceptorChain.chainAndInvoke(InvocationInterceptorChain.java:45)
at org.junit.jupiter.engine.execution.InvocationInterceptorChain.invoke(InvocationInterceptorChain.java:37)
at org.junit.jupiter.engine.execution.ExecutableInvoker.invoke(ExecutableInvoker.java:104)
at org.junit.jupiter.engine.execution.ExecutableInvoker.invoke(ExecutableInvoker.java:98)
at org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor.lambda$invokeTestMethod$7(TestMethodTestDescriptor.java:214)
at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
at org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor.invokeTestMethod(TestMethodTestDescriptor.java:210)
at org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor.execute(TestMethodTestDescriptor.java:135)
at org.junit.jupiter.engine.descriptor.TestMethodTestDescriptor.execute(TestMethodTestDescriptor.java:66)
at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$6(NodeTestTask.java:151)
at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$8(NodeTestTask.java:141)
at org.junit.platform.engine.support.hierarchical.Node.around(Node.java:137)
at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$9(NodeTestTask.java:139)
at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
at org.junit.platform.engine.support.hierarchical.NodeTestTask.executeRecursively(NodeTestTask.java:138)
at org.junit.platform.engine.support.hierarchical.NodeTestTask.execute(NodeTestTask.java:95)
at java.util.ArrayList.forEach(ArrayList.java:1259)
at org.junit.platform.engine.support.hierarchical.SameThreadHierarchicalTestExecutorService.invokeAll(SameThreadHierarchicalTestExecutorService.java:41)
at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$6(NodeTestTask.java:155)
at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$8(NodeTestTask.java:141)
at org.junit.platform.engine.support.hierarchical.Node.around(Node.java:137)
at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$9(NodeTestTask.java:139)
at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
at org.junit.platform.engine.support.hierarchical.NodeTestTask.executeRecursively(NodeTestTask.java:138)
at org.junit.platform.engine.support.hierarchical.NodeTestTask.execute(NodeTestTask.java:95)
at java.util.ArrayList.forEach(ArrayList.java:1259)
at org.junit.platform.engine.support.hierarchical.SameThreadHierarchicalTestExecutorService.invokeAll(SameThreadHierarchicalTestExecutorService.java:41)
at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$6(NodeTestTask.java:155)
at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$8(NodeTestTask.java:141)
at org.junit.platform.engine.support.hierarchical.Node.around(Node.java:137)
at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$executeRecursively$9(NodeTestTask.java:139)
at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
at org.junit.platform.engine.support.hierarchical.NodeTestTask.executeRecursively(NodeTestTask.java:138)
at org.junit.platform.engine.support.hierarchical.NodeTestTask.execute(NodeTestTask.java:95)
at org.junit.platform.engine.support.hierarchical.SameThreadHierarchicalTestExecutorService.submit(SameThreadHierarchicalTestExecutorService.java:35)
at org.junit.platform.engine.support.hierarchical.HierarchicalTestExecutor.execute(HierarchicalTestExecutor.java:57)
at org.junit.platform.engine.support.hierarchical.HierarchicalTestEngine.execute(HierarchicalTestEngine.java:54)
at org.junit.platform.launcher.core.EngineExecutionOrchestrator.execute(EngineExecutionOrchestrator.java:107)
at org.junit.platform.launcher.core.EngineExecutionOrchestrator.execute(EngineExecutionOrchestrator.java:88)
at org.junit.platform.launcher.core.EngineExecutionOrchestrator.lambda$execute$0(EngineExecutionOrchestrator.java:54)
at org.junit.platform.launcher.core.EngineExecutionOrchestrator.withInterceptedStreams(EngineExecutionOrchestrator.java:67)
at org.junit.platform.launcher.core.EngineExecutionOrchestrator.execute(EngineExecutionOrchestrator.java:52)
at org.junit.platform.launcher.core.DefaultLauncher.execute(DefaultLauncher.java:114)
at org.junit.platform.launcher.core.DefaultLauncher.execute(DefaultLauncher.java:86)
at org.junit.platform.launcher.core.DefaultLauncherSession$DelegatingLauncher.execute(DefaultLauncherSession.java:86)
at org.junit.platform.launcher.core.SessionPerRequestLauncher.execute(SessionPerRequestLauncher.java:53)
at org.apache.maven.surefire.junitplatform.JUnitPlatformProvider.invokeAllTests(JUnitPlatformProvider.java:150)
at org.apache.maven.surefire.junitplatform.JUnitPlatformProvider.invoke(JUnitPlatformProvider.java:124)
at org.apache.maven.surefire.booter.ForkedBooter.invokeProviderInSameClassLoader(ForkedBooter.java:384)
at org.apache.maven.surefire.booter.ForkedBooter.runSuitesInProcess(ForkedBooter.java:345)
at org.apache.maven.surefire.booter.ForkedBooter.execute(ForkedBooter.java:126)
at org.apache.maven.surefire.booter.ForkedBooter.main(ForkedBooter.java:418)
```
**After**
```
[INFO] Running org.apache.uniffle.common.RssShuffleUtilsTest
[INFO] Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.284 s - in org.apache.uniffle.common.RssShuffleUtilsTest
```
--
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: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] codecov-commenter commented on pull request #203: [ISSUE-144] Fix flaky test `RssShuffleUtilsTest#testDestroyDirectByteBuffer`
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #203:
URL: https://github.com/apache/incubator-uniffle/pull/203#issuecomment-1238025220
# [Codecov](https://codecov.io/gh/apache/incubator-uniffle/pull/203?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#203](https://codecov.io/gh/apache/incubator-uniffle/pull/203?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (d95d3aa) into [master](https://codecov.io/gh/apache/incubator-uniffle/commit/07f70ed872b87107fc7028577bd9e66d8349fd6c?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (07f70ed) will **decrease** coverage by `0.04%`.
> The diff coverage is `100.00%`.
> :exclamation: Current head d95d3aa differs from pull request most recent head d8f8cb9. Consider uploading reports for the commit d8f8cb9 to get more accurate results
```diff
@@ Coverage Diff @@
## master #203 +/- ##
============================================
- Coverage 58.48% 58.44% -0.05%
- Complexity 1277 1278 +1
============================================
Files 158 158
Lines 8463 8458 -5
Branches 785 785
============================================
- Hits 4950 4943 -7
- Misses 3259 3262 +3
+ Partials 254 253 -1
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-uniffle/pull/203?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...ava/org/apache/uniffle/common/RssShuffleUtils.java](https://codecov.io/gh/apache/incubator-uniffle/pull/203/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS91bmlmZmxlL2NvbW1vbi9Sc3NTaHVmZmxlVXRpbHMuamF2YQ==) | `94.44% <100.00%> (-1.21%)` | :arrow_down: |
| [.../apache/uniffle/coordinator/CoordinatorServer.java](https://codecov.io/gh/apache/incubator-uniffle/pull/203/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Y29vcmRpbmF0b3Ivc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL3VuaWZmbGUvY29vcmRpbmF0b3IvQ29vcmRpbmF0b3JTZXJ2ZXIuamF2YQ==) | `59.57% <0.00%> (-5.32%)` | :arrow_down: |
| [...org/apache/uniffle/server/ShuffleFlushManager.java](https://codecov.io/gh/apache/incubator-uniffle/pull/203/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c2VydmVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS91bmlmZmxlL3NlcnZlci9TaHVmZmxlRmx1c2hNYW5hZ2VyLmphdmE=) | `78.33% <0.00%> (+1.66%)` | :arrow_up: |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
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: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] LuciferYang commented on pull request #203: [ISSUE-144] Fix flaky test `RssShuffleUtilsTest#testDestroyDirectByteBuffer`
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on PR #203:
URL: https://github.com/apache/incubator-uniffle/pull/203#issuecomment-1238051337
thanks @jerqi
--
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: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] LuciferYang commented on pull request #203: [WIP][ISSUE-144] Simplify implementation of `destroyDirectByteBuffer` method and enhance the stability of related test
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on PR #203:
URL: https://github.com/apache/incubator-uniffle/pull/203#issuecomment-1237979834
checkstyle may failed , will fix later
--
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: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] jerqi merged pull request #203: [ISSUE-144] Fix flaky test `RssShuffleUtilsTest#testDestroyDirectByteBuffer`
Posted by GitBox <gi...@apache.org>.
jerqi merged PR #203:
URL: https://github.com/apache/incubator-uniffle/pull/203
--
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: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] LuciferYang commented on a diff in pull request #203: [WIP][ISSUE-144] Simplify implementation of `destroyDirectByteBuffer` method and enhance the stability of related test
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #203:
URL: https://github.com/apache/incubator-uniffle/pull/203#discussion_r963578295
##########
common/src/main/java/org/apache/uniffle/common/RssShuffleUtils.java:
##########
@@ -73,15 +72,9 @@ public static ByteBuffer decompressData(ByteBuffer data, int uncompressLength, b
*
*/
public static void destroyDirectByteBuffer(ByteBuffer toBeDestroyed)
- throws IllegalArgumentException, IllegalAccessException,
- InvocationTargetException, SecurityException, NoSuchMethodException {
+ throws IllegalArgumentException, SecurityException {
Review Comment:
revert this change due to Java compatibility
--
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: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org
[GitHub] [incubator-uniffle] LuciferYang commented on a diff in pull request #203: [WIP][ISSUE-144] Fix flaky test `RssShuffleUtilsTest#testDestroyDirectByteBuffer`
Posted by GitBox <gi...@apache.org>.
LuciferYang commented on code in PR #203:
URL: https://github.com/apache/incubator-uniffle/pull/203#discussion_r963585918
##########
common/src/test/java/org/apache/uniffle/common/RssShuffleUtilsTest.java:
##########
@@ -57,9 +60,19 @@ public void testDestroyDirectByteBuffer() throws Exception {
byteBuffer.put(b);
}
byteBuffer.flip();
+
+ // Get valid native pointer through `address` in `DirectByteBuffer`
+ Unsafe unsafe = getUnsafe();
+ long addressInByteBuffer = address(byteBuffer);
+ long originalAddress = unsafe.getAddress(addressInByteBuffer);
+
RssShuffleUtils.destroyDirectByteBuffer(byteBuffer);
+
// The memory may not be released fast enough.
- Thread.sleep(200);
+ // If native pointer changes, `address` in `DirectByteBuffer` is invalid
+ while (unsafe.getAddress(addressInByteBuffer) == originalAddress) {
Review Comment:
```java
/**
* Fetches a native pointer from a given memory address. If the address is
* zero, or does not point into a block obtained from {@link
* #allocateMemory}, the results are undefined.
*
* <p> If the native pointer is less than 64 bits wide, it is extended as
* an unsigned number to a Java long. The pointer may be indexed by any
* given byte offset, simply by adding that offset (as a simple integer) to
* the long representing the pointer. The number of bytes actually read
* from the target address maybe determined by consulting {@link
* #addressSize}.
*
* @see #allocateMemory
*/
public native long getAddress(long address);
```
--
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: issues-unsubscribe@uniffle.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org