[MINOR] Migrate RankValue to the package of the common class by smallzhongfeng · Pull Request #265 · apache/uniffle · GitHub
Skip to content

[MINOR] Migrate RankValue to the package of the common class#265

Merged
roryqi merged 3 commits into
apache:masterfrom
smallzhongfeng:Migrate-RankValue
Nov 18, 2022
Merged

[MINOR] Migrate RankValue to the package of the common class#265
roryqi merged 3 commits into
apache:masterfrom
smallzhongfeng:Migrate-RankValue

Conversation

@smallzhongfeng

Copy link
Copy Markdown
Contributor

What changes were proposed in this pull request?

More standardized, and the class can be extended in the future.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

No need.

@codecov-commenter

codecov-commenter commented Oct 15, 2022

Copy link
Copy Markdown

@smallzhongfeng

Copy link
Copy Markdown
Contributor Author

Please cc, just a minor change. @jerqi

@roryqi

roryqi commented Oct 16, 2022

Copy link
Copy Markdown
Contributor

Could you give me a concrete example? Why do we need this pr?

@smallzhongfeng

Copy link
Copy Markdown
Contributor Author

Because this static class is currently in LowestIOSampleCostSelectStorageStrategy, but AppBalanceSelectStorageStrategy will also use it, and other selection strategy may use it later, it will be common to set it as a normal class. In addition, sorting is required when selecting the optimal storage path. Sorting depends on this object. If other storage needs to rely on this RankValue, and more comparative attributes need to be added, it will be clearer to define these attributes in a separate class. WDYT ? @jerqi

@roryqi

roryqi commented Oct 17, 2022

Copy link
Copy Markdown
Contributor

Why do we migrate RankValue to the package of the common class if shuffle server and client won't use RankValue.

@smallzhongfeng

Copy link
Copy Markdown
Contributor Author

I see what you mean, but I think it is necessary to set it as a separate class. Put it under the coordinator package ?

@roryqi

roryqi commented Oct 17, 2022

Copy link
Copy Markdown
Contributor

I see what you mean, but I think it is necessary to set it as a separate class. Put it under the coordinator package ?

Coordinator package is better.

@roryqi

roryqi commented Nov 12, 2022

Copy link
Copy Markdown
Contributor

@smallzhongfeng Will you continue this pr?

@smallzhongfeng

Copy link
Copy Markdown
Contributor Author

@roryqi roryqi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, thanks @smallzhongfeng

@roryqi roryqi merged commit 2df7594 into apache:master Nov 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants