[#2111] improvement(client): cleanup the useless shuffle server clients when unregister shuffle by xianjingfeng · Pull Request #2112 · apache/uniffle

xianjingfeng · 2024-09-11T07:54:20Z

What changes were proposed in this pull request?

Cleanup useless the shuffle server clients when unregister shuffle

Why are the changes needed?

Fix: #2111

Does this PR introduce any user-facing change?

No.

How was this patch tested?

UT

github-actions · 2024-09-11T08:19:22Z

Test Results

3 034 files + 152 3 034 suites +152 6h 43m 35s ⏱️ + 46m 53s
1 179 tests + 160 1 178 ✅ + 160 1 💤 ±0 0 ❌ ±0
14 937 runs +2 069 14 922 ✅ +2 069 15 💤 ±0 0 ❌ ±0

Results for commit 8729ecd. ± Comparison against base commit d170004.

This pull request removes 30 and adds 190 tests. Note that renamed tests count towards both.

org.apache.uniffle.common.records.RecordsReaderWriterTest ‑ testWriteAndReadRecordFile1{String, File}[1]
org.apache.uniffle.common.records.RecordsReaderWriterTest ‑ testWriteAndReadRecordFile1{String, File}[2]
org.apache.uniffle.common.records.RecordsReaderWriterTest ‑ testWriteAndReadRecordFile2{String, File}[1]
org.apache.uniffle.common.records.RecordsReaderWriterTest ‑ testWriteAndReadRecordFile2{String, File}[2]
org.apache.uniffle.common.records.RecordsReaderWriterTest ‑ testWriteAndReadRecordFile3{String, File}[1]
org.apache.uniffle.common.records.RecordsReaderWriterTest ‑ testWriteAndReadRecordFile3{String, File}[2]
org.apache.uniffle.common.records.RecordsReaderWriterTest ‑ testWriteAndReadRecordFile4{String, File}[1]
org.apache.uniffle.common.records.RecordsReaderWriterTest ‑ testWriteAndReadRecordFile4{String, File}[2]
org.apache.uniffle.common.serializer.PartialInputStreamTest ‑ testReadFileInputStream
org.apache.uniffle.common.serializer.PartialInputStreamTest ‑ testReadMemroyInputStream
…

org.apache.hadoop.mapred.SortWriteBufferManagerTest ‑ testWriteNormalWithRemoteMerge
org.apache.hadoop.mapred.SortWriteBufferManagerTest ‑ testWriteNormalWithRemoteMergeAndCombine
org.apache.hadoop.mapreduce.task.reduce.RMRssShuffleTest ‑ testReadShuffleWithCombine
org.apache.hadoop.mapreduce.task.reduce.RMRssShuffleTest ‑ testReadShuffleWithoutCombine
org.apache.spark.shuffle.DelegationRssShuffleManagerTest ‑ testDefaultIncludeExcludeProperties
org.apache.spark.shuffle.DelegationRssShuffleManagerTest ‑ testExcludeProperties
org.apache.spark.shuffle.DelegationRssShuffleManagerTest ‑ testIncludeProperties
org.apache.spark.shuffle.handle.MutableShuffleHandleInfoTest ‑ testUpdateAssignmentOnPartitionSplit
org.apache.tez.runtime.library.common.shuffle.orderedgrouped.RMRssShuffleTest ‑ testReadMultiPartitionShuffleData
org.apache.tez.runtime.library.common.shuffle.orderedgrouped.RMRssShuffleTest ‑ testReadShuffleData
…

♻️ This comment has been updated with latest results.

roryqi · 2024-09-13T10:40:03Z

What will it happen if we don't have this patch?

xianjingfeng · 2024-09-14T01:53:27Z

What will it happen if we don't have this patch?

If there are many stages in a application, the client will keep the clients for all shuffle servers, which will waste memory.

roryqi · 2024-09-14T03:32:12Z

What will it happen if we don't have this patch?

If there are many stages in a application, the client will keep the clients for all shuffle servers, which will waste memory.

One server may be used by multiple stages. We can't remove it if only one stage is finished.

xianjingfeng · 2024-09-14T09:05:29Z

What will it happen if we don't have this patch?

If there are many stages in a application, the client will keep the clients for all shuffle servers, which will waste memory.

One server may be used by multiple stages. We can't remove it if only one stage is finished.

Right, not all the shuffle servers will be deleted in this PR

zuston

Emm. I think we should have the more graceful way to close the shuffle-server clients.

For my sight, I hope the closing way should be optional, which can be implemented by the pluggable closable policies.

zuston · 2024-09-18T09:34:38Z

internal-client/src/main/java/org/apache/uniffle/client/factory/ShuffleServerClientFactory.java

    return serverToClients.get(shuffleServerInfo);
  }

+  public synchronized void cleanUselessShuffleServerClients(


Is is possible that the another thread to hold the removing client?

I think it is impossible, if another thread hold the removing client, the shuffleServerInfo of the removing client will always be in shuffleServerInfoMap.

xianjingfeng · 2024-09-19T03:10:16Z

I hope the closing way should be optional,

+1

which can be implemented by the pluggable closable policies.

What are the possible policies?

roryqi · 2024-12-16T03:05:25Z

@zuston gently ping.

xianjingfeng · 2025-04-09T08:30:13Z

@zuston gently ping.

zuston · 2025-04-09T13:10:52Z

internal-client/src/main/java/org/apache/uniffle/client/factory/ShuffleServerClientFactory.java

    return serverToClients.get(shuffleServerInfo);
  }

+  public synchronized void cleanUselessShuffleServerClients(


This method impl is not straightforward, I hope the impl like the following code:

public synchronized void closeClients(List<ShuffleServerInfo> candidates) { // .... }

zuston · 2025-04-09T13:11:27Z

Sorry the late review and I think i missed this thread. @xianjingfeng

zuston · 2025-04-11T02:02:46Z

internal-client/src/main/java/org/apache/uniffle/client/factory/ShuffleServerClientFactory.java

    return serverToClients.get(shuffleServerInfo);
  }

+  public synchronized void closeClients(Set<ShuffleServerInfo> candidates) {


I think you misunderstand my thought. I hope the candidates are the deleted client candidates rather than being excluded clients.

public synchronized void closeClients(Set<ShuffleServerInfo> candidates) { clients.remove_all(candidates) }

We need use synchronized outside if we find the candidates outside of this method. As follow

ShuffleServerClientFactory shuffleServerClientFactory = ShuffleServerClientFactory.getInstance(); synchronized (shuffleServerClientFactory) { Set<ShuffleServerInfo> candidates = shuffleServerClientFactory.findClosableClients(getAllShuffleServers()); shuffleServerClientFactory.closeClients(candidates); }

It feels worse than the original implementation.

From my thought, all clients are managed by the shuffleServerClientFactory, if in this class, we could maintain a structure to record the every client reference count num. if the client reference count is 0, then we could release it.

If so, the +1 operation could be happened on the getShuffleServerClient. And -1 should be happened on the removing in shuffleWriteClientImpl.

How about this way?

If so, the +1 operation could be happened on the getShuffleServerClient. And -1 should be happened on the removing in shuffleWriteClientImpl.

I think this can't be achieved. To do this, we must invoke getShuffleServerClient before we invoke the RPC interface every time and -1 after each call to the RPC interface.

This reverts commit f6485f7.

summaryzb

How about get the client through a guava cache with expire time setting, maybe it's more common without caring about whether the corresponding server is useless or availeble

xianjingfeng · 2025-04-14T06:55:30Z

How about get the client through a guava cache with expire time setting, maybe it's more common without caring about whether the corresponding server is useless or availeble

This solution will have problems if the client is only retrieved from the cache once when it is used in the future.

xianjingfeng · 2025-04-14T07:49:11Z

I think I should close this PR. Because I find ShuffleServerClientFactory#getShuffleServerClient needs to be optimized more than it. #2441

cleanup shuffle server client when unregister shuffle

144f723

xianjingfeng requested review from rickyma and zuston September 11, 2024 07:54

xianjingfeng requested a review from roryqi September 13, 2024 10:16

xianjingfeng changed the title ~~[#2111] improvement(client): cleanup shuffle server client when unregister shuffle~~ [#2111] improvement(client): cleanup useless shuffle server client when unregister shuffle Sep 14, 2024

xianjingfeng changed the title ~~[#2111] improvement(client): cleanup useless shuffle server client when unregister shuffle~~ [#2111] improvement(client): cleanup useless the shuffle server clients when unregister shuffle Sep 14, 2024

nit

3e4dbf9

xianjingfeng changed the title ~~[#2111] improvement(client): cleanup useless the shuffle server clients when unregister shuffle~~ [#2111] improvement(client): cleanup the useless shuffle server clients when unregister shuffle Sep 14, 2024

zuston reviewed Sep 18, 2024

View reviewed changes

zuston reviewed Apr 9, 2025

View reviewed changes

nit

f6485f7

zuston reviewed Apr 11, 2025

View reviewed changes

Revert "nit"

8729ecd

This reverts commit f6485f7.

summaryzb reviewed Apr 11, 2025

View reviewed changes

xianjingfeng closed this Apr 14, 2025

xianjingfeng mentioned this pull request Apr 14, 2025

[#2441] improvement(client): remove synchronized in ShuffleServerClientFactory#getShuffleServerClient #2442

Merged

Conversation

xianjingfeng commented Sep 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

github-actions bot commented Sep 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results

Uh oh!

roryqi commented Sep 13, 2024

Uh oh!

xianjingfeng commented Sep 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

roryqi commented Sep 14, 2024

Uh oh!

xianjingfeng commented Sep 14, 2024

Uh oh!

zuston left a comment

Choose a reason for hiding this comment

Uh oh!

zuston Sep 18, 2024

Choose a reason for hiding this comment

Uh oh!

xianjingfeng Sep 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xianjingfeng commented Sep 19, 2024

Uh oh!

roryqi commented Dec 16, 2024

Uh oh!

xianjingfeng commented Apr 9, 2025

Uh oh!

zuston Apr 9, 2025

Choose a reason for hiding this comment

Uh oh!

zuston commented Apr 9, 2025

Uh oh!

zuston Apr 11, 2025

Choose a reason for hiding this comment

Uh oh!

xianjingfeng Apr 11, 2025

Choose a reason for hiding this comment

Uh oh!

zuston Apr 11, 2025

Choose a reason for hiding this comment

Uh oh!

xianjingfeng Apr 11, 2025

Choose a reason for hiding this comment

Uh oh!

summaryzb left a comment

Choose a reason for hiding this comment

Uh oh!

xianjingfeng commented Apr 14, 2025

Uh oh!

xianjingfeng commented Apr 14, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

xianjingfeng commented Sep 11, 2024 •

edited

Loading

github-actions bot commented Sep 11, 2024 •

edited

Loading

xianjingfeng commented Sep 14, 2024 •

edited

Loading

xianjingfeng Sep 19, 2024 •

edited

Loading