[MINOR] Server heartbeat to coordinator while unregister shuffle

[maobaolong]

What changes were proposed in this pull request?

Server send heartbeat to coordinator while receive unregister shuffle request.

Why are the changes needed?

Without this PR, server could not heartbeat the updated app info after unregister this app, so the coordinator and dashboard could display the outdate information.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Tested by a tiny spark job executed by spark-shell

• shell command line

bin/spark-shell  --master  spark://localhost:7077  --deploy-mode client --conf spark.rss.coordinator.quorum=localhost:19999   --conf spark.shuffle.manager=org.apache.spark.shuffle.RssShuffleManager  --conf spark.rss.storage.type=LOCALFILE --conf spark.serializer=org.apache.spark.serializer.KryoSerializer --conf spark.rss.test.mode.enable=true --conf spark.rss.client.type=GRPC_NETTY --conf spark.default.parallelism=16  -i test.scala

• test.scala

val data = sc.parallelize(Seq(("A", 1), ("B", 2), ("C", 3), ("A", 4), ("B", 5), ("A", 6), ("A", 7),("A", 7), ("A", 7), ("A", 7), ("A", 7), ("A", 7), ("A", 7), ("A", 7), ("A", 7), ("A", 7), ("A", 7), ("A", 7), ("A", 7)));
val result = data.reduceByKey(_ + _);
result.collect().foreach(println);
System.exit(0);

[github-actions]

Test Results

 2 950 files   -  6   2 950 suites   - 6   6h 14m 38s ⏱️ - 12m 37s
 1 092 tests ± 0   1 089 ✅  -  1   2 💤 ±0  1 ❌ +1 
13 673 runs   - 12  13 642 ✅  - 13  30 💤 ±0  1 ❌ +1 

For more details on these failures, see this check.

Results for commit 770177a. ± Comparison against base commit c1bfa04.

♻️ This comment has been updated with latest results.

[zuston]

I hope the heartbeat could not be invoked by other thread like the unrelated unregister operations. If you want the latest info, you can decrease the heartbeat interval of server -> coordinator. WDYT? @maobaolong

[maobaolong]

@zuston The affect of decrease interval can make it better but not a fundamental solution, the latest info could be lost also, and the frequency heartbeat could make Coordinator busy.

And could you elaborate a little more on what concern from your side if heartbeat invoked by another thread?

[zuston]

@zuston The affect of decrease interval can make it better but not a fundamental solution, the latest info could be lost also, and the frequency heartbeat could make Coordinator busy.

And could you elaborate a little more on what concern from your side if heartbeat invoked by another thread?

External client operation will control the frequency of internal heartbeat, this is unreasonable and danger.

[maobaolong]

@zuston Hmm, well, is there another way to send the latest info?

Without the latest info, the app information is not completed. Do you think it could be better if I create a new RPC method to talk to coordinator for the latest info?

[maobaolong]

@zuston Thanks for the discussion offline. I think I need to introduce the motivation of this PR.

The motivation of this PR aimed to resolve the scenario for our regression test, it is a simple test with tiny shuffle data as the description given, without this PR, the coordinator cannot collect any application information.

For the production scenario, it could be a little better, since lots of app have a huge mount of shuffle data, but the risky of lost last heartbeat still exist.

As a tradeoff, I add a configure option(default to false to keep the original behavior), it can be set to true if we need to get the exact information of application, and keep to false by default.

[maobaolong]

ping @zuston

[zuston]

How about making this option rename to rss.server.heartbeatReportOnUnregisterEnabled ?

[maobaolong]

done

[maobaolong]

@zuston Thanks for your suggestion, renamed by last commit . PTAL