Open maobaolong opened 1 month ago
2 926 files +29 2 926 suites +29 6h 16m 45s :stopwatch: + 35m 38s 1 049 tests ± 0 1 047 :white_check_mark: + 2 2 :zzz: ±0 0 :x: - 1 13 045 runs +62 13 015 :white_check_mark: +64 30 :zzz: ±0 0 :x: - 1
Results for commit 2cac65a1. ± Comparison against base commit 2b70eb4d.
:recycle: This comment has been updated with latest results.
@jerqi Would you like to take a look at this PR? Thanks!
I have already had server version, you can see Constants::SHUFFLE_SERVER_VERSION
@jerqi Thanks for this remind, after go through the code related to SHUFFLE_SERVER_VERSION
, I have some primary knowledge about this.
In conclusion, I guess the motivation of the SHUFFLE_SERVER_VERSION
aimed to select the same version/tag server for client. String type tag/version make easy to understand version, but hard to extend, if we extend a new feature base on ss_v5
, old client could not use ss_v5
as tag to get assignment, even the new feature include ss_v5
feature.
But this motivation of ServiceVersion
is aimed to make client adopt to the servers with different serviceVersion
, to make them all works.
@jerqi Thanks for this remind, after go through the code related to
SHUFFLE_SERVER_VERSION
, I have some primary knowledge about this.
- SHUFFLE_SERVER_VERSION is used as a tag of server
- Coordinator select the servers for client follow the tag filter from client
- Metrics related
In conclusion, I guess the motivation of the
SHUFFLE_SERVER_VERSION
aimed to select the same version/tag server for client. String type tag/version make easy to understand version, but hard to extend, if we extend a new feature base onss_v5
, old client could not usess_v5
as tag to get assignment, even the new feature includess_v5
feature.But this motivation of
ServiceVersion
is aimed to make client adopt to the servers with differentserviceVersion
, to make them all work
NO, if we give uncompatible update, we should change the version.
What changes were proposed in this pull request?
getShuffleResult
andgetShuffleResultForMultiPart
.Why are the changes needed?
Fix: #2212
Does this PR introduce any user-facing change?
No.
How was this patch tested?
Test locally