Skip to content

[#1211] fix: Unexpectedly removing resources even though the App has re-registered shuffle later. - #1212

Merged
zuston merged 3 commits into
apache:masterfrom
zhuyaogai:fix_remove_resources
Sep 27, 2023
Merged

[#1211] fix: Unexpectedly removing resources even though the App has re-registered shuffle later.#1212
zuston merged 3 commits into
apache:masterfrom
zhuyaogai:fix_remove_resources

Conversation

@zhuyaogai

@zhuyaogai zhuyaogai commented Sep 24, 2023

Copy link
Copy Markdown
Contributor

What changes were proposed in this pull request?

Check again if the App has expired when removing resources.

Why are the changes needed?

Fix: #1211

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Add more tests.

@codecov-commenter

codecov-commenter commented Sep 24, 2023 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #1212 (8978079) into master (10e8e3d) will increase coverage by 1.20%.
Report is 7 commits behind head on master.
The diff coverage is 86.66%.

@@             Coverage Diff              @@
##             master    #1212      +/-   ##
============================================
+ Coverage     53.68%   54.88%   +1.20%     
- Complexity     2588     2604      +16     
============================================
  Files           391      372      -19     
  Lines         22425    20135    -2290     
  Branches       1875     1886      +11     
============================================
- Hits          12038    11051     -987     
+ Misses         9682     8447    -1235     
+ Partials        705      637      -68
Files Coverage Δ
.../org/apache/uniffle/server/ShuffleTaskManager.java 75.89% <86.66%> (+0.01%) ⬆️

... and 25 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@zhuyaogai

zhuyaogai commented Sep 27, 2023

Copy link
Copy Markdown
Contributor Author

@zuston Could you review code for me if you are available? Thanks!

@zuston
zuston self-requested a review September 27, 2023 08:30
zuston
zuston reviewed Sep 27, 2023
View reviewed changes

@zuston zuston left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isAppExpired looks good to me. But I don't get the point about the appLocks .

@zhuyaogai

zhuyaogai commented Sep 27, 2023

Copy link
Copy Markdown
Contributor Author

@zuston As we discussed before, we cannot predict the execution order of registerShuffle and removeReources. Assume the following execution order.

  1. In removeReources method, invoke isAppExpired method returns false.
  2. In registerShuffle method, it registers some relevant application information.
  3. In removeReources method, it removes all relevant application information.
  4. In client side, when invoking requireBuffer will also return NO_REGISTER status code...

Please correct me if I'm wrong or you can provide better suggestions. Thanks!

@zhuyaogai
zhuyaogai requested a review from zuston September 27, 2023 08:54
zuston
zuston approved these changes Sep 27, 2023
View reviewed changes

@zuston zuston left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make sense.

@zuston
zuston merged commit be8d000 into apache:master Sep 27, 2023
@connorlwilkes

connorlwilkes commented Oct 3, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

This seems to have broken things for me. The call to the guava cache throws an execption in the shuffle server:

Exception in thread "Grpc-0" java.lang.NoClassDefFoundError: org/apache/uniffle/guava/util/concurrent/internal/InternalFutureFailureAccess
        at java.base/java.lang.ClassLoader.defineClass1(Native Method)
        at java.base/java.lang.ClassLoader.defineClass(ClassLoader.java:1012)
        at java.base/java.security.SecureClassLoader.defineClass(SecureClassLoader.java:150)
        at java.base/jdk.internal.loader.BuiltinClassLoader.defineClass(BuiltinClassLoader.java:862)
        at java.base/jdk.internal.loader.BuiltinClassLoader.findClassOnClassPathOrNull(BuiltinClassLoader.java:760)
        at java.base/jdk.internal.loader.BuiltinClassLoader.loadClassOrNull(BuiltinClassLoader.java:681)
        at java.base/jdk.internal.loader.BuiltinClassLoader.loadClass(BuiltinClassLoader.java:639)
        at java.base/jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(ClassLoaders.java:188)
        at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:520)
        at java.base/java.lang.ClassLoader.defineClass1(Native Method)
        at java.base/java.lang.ClassLoader.defineClass(ClassLoader.java:1012)
        at java.base/java.security.SecureClassLoader.defineClass(SecureClassLoader.java:150)
        at java.base/jdk.internal.loader.BuiltinClassLoader.defineClass(BuiltinClassLoader.java:862)
        at java.base/jdk.internal.loader.BuiltinClassLoader.findClassOnClassPathOrNull(BuiltinClassLoader.java:760)
        at java.base/jdk.internal.loader.BuiltinClassLoader.loadClassOrNull(BuiltinClassLoader.java:681)
        at java.base/jdk.internal.loader.BuiltinClassLoader.loadClass(BuiltinClassLoader.java:639)
        at java.base/jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(ClassLoaders.java:188)
        at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:520)
        at java.base/java.lang.ClassLoader.defineClass1(Native Method)
        at java.base/java.lang.ClassLoader.defineClass(ClassLoader.java:1012)
        at java.base/java.security.SecureClassLoader.defineClass(SecureClassLoader.java:150)
        at java.base/jdk.internal.loader.BuiltinClassLoader.defineClass(BuiltinClassLoader.java:862)
        at java.base/jdk.internal.loader.BuiltinClassLoader.findClassOnClassPathOrNull(BuiltinClassLoader.java:760)
        at java.base/jdk.internal.loader.BuiltinClassLoader.loadClassOrNull(BuiltinClassLoader.java:681)
        at java.base/jdk.internal.loader.BuiltinClassLoader.loadClass(BuiltinClassLoader.java:639)
        at java.base/jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(ClassLoaders.java:188)
        at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:520)
        at org.apache.uniffle.guava.cache.LocalCache$LoadingValueReference.<init>(LocalCache.java:3476)
        at org.apache.uniffle.guava.cache.LocalCache$LoadingValueReference.<init>(LocalCache.java:3480)
        at org.apache.uniffle.guava.cache.LocalCache$Segment.lockedGetOrLoad(LocalCache.java:2138)
        at org.apache.uniffle.guava.cache.LocalCache$Segment.get(LocalCache.java:2049)
        at org.apache.uniffle.guava.cache.LocalCache.get(LocalCache.java:3966)
        at org.apache.uniffle.guava.cache.LocalCache$LocalManualCache.get(LocalCache.java:4863)
        at org.apache.uniffle.server.ShuffleTaskManager.getAppLock(ShuffleTaskManager.java:193)
        at org.apache.uniffle.server.ShuffleTaskManager.registerShuffle(ShuffleTaskManager.java:226)
        at org.apache.uniffle.server.ShuffleServerGrpcService.registerShuffle(ShuffleServerGrpcService.java:163)
        at org.apache.uniffle.proto.ShuffleServerGrpc$MethodHandlers.invoke(ShuffleServerGrpc.java:1033)
        at io.grpc.stub.ServerCalls$UnaryServerCallHandler$UnaryServerCallListener.onHalfClose(ServerCalls.java:182)
        at io.grpc.PartialForwardingServerCallListener.onHalfClose(PartialForwardingServerCallListener.java:35)
        at io.grpc.ForwardingServerCallListener.onHalfClose(ForwardingServerCallListener.java:23)
        at io.grpc.internal.ServerCallImpl$ServerStreamListenerImpl.halfClosed(ServerCallImpl.java:352)
        at io.grpc.internal.ServerImpl$JumpToApplicationThreadServerStreamListener$1HalfClosed.runInContext(ServerImpl.java:866)
        at io.grpc.internal.ContextRunnable.run(ContextRunnable.java:37)
        at io.grpc.internal.SerializingExecutor.run(SerializingExecutor.java:133)
        at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1136)
        at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:635)
        at java.base/java.lang.Thread.run(Thread.java:833)
Caused by: java.lang.ClassNotFoundException: org.apache.uniffle.guava.util.concurrent.internal.InternalFutureFailureAccess
        at java.base/jdk.internal.loader.BuiltinClassLoader.loadClass(BuiltinClassLoader.java:641)
        at java.base/jdk.internal.loader.ClassLoaders$AppClassLoader.loadClass(ClassLoaders.java:188)
        at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:520)
        ... 47 more

Reverting the commit fixes the issue

@zuston

roryqi pushed a commit that referenced this pull request Oct 17, 2023
@zhuyaogai @roryqi
[#1211] fix(server): unexpectedly removing resources when app has re-…
096c4b9 
…registered shuffle later (#1212)

### What changes were proposed in this pull request?

1. check again if the App has expired when removing resources.
2. add the lock of app scope to keep the sequence of remove and register operations

### Why are the changes needed?

Fix: #1211

### Does this PR introduce _any_ user-facing change?

No.

### How was this patch tested?

Add more tests.
@zuston zuston mentioned this pull request Feb 7, 2024
Merged
@xianjingfeng xianjingfeng mentioned this pull request Apr 21, 2025
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zuston zuston zuston approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Bug] Unexpectedly removing resources even though the App has re-registered shuffle later.

4 participants

@zhuyaogai @codecov-commenter @connorlwilkes @zuston