New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[FEATURE BRANCH] More work on shuffle sharding utils #81182
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,37 +18,51 @@ package shufflesharding | |
|
||
import ( | ||
"errors" | ||
"fmt" | ||
"math" | ||
"strings" | ||
) | ||
|
||
const maxHashBits = 60 | ||
|
||
// ValidateParameters can validate parameters for shuffle sharding | ||
// in a fast but approximate way, including deckSize and handSize | ||
// Algorithm: maxHashBits >= bits(deckSize^handSize) | ||
func ValidateParameters(deckSize, handSize int32) bool { | ||
if handSize <= 0 || deckSize <= 0 || handSize > deckSize { | ||
return false | ||
// ValidateParameters finds errors in the parameters for shuffle | ||
// sharding. Returns a slice for which `len()` is 0 if and only if | ||
// there are no errors. The entropy requirement is evaluated in a | ||
// fast but approximate way: bits(deckSize^handSize). | ||
func ValidateParameters(deckSize, handSize int) (errs []string) { | ||
if handSize <= 0 { | ||
errs = append(errs, "handSize is not positive") | ||
} | ||
|
||
return math.Log2(float64(deckSize))*float64(handSize) <= maxHashBits | ||
if deckSize <= 0 { | ||
errs = append(errs, "deckSize is not positive") | ||
} | ||
if len(errs) > 0 { | ||
return | ||
} | ||
if handSize > deckSize { | ||
return []string{"handSize is greater than deckSize"} | ||
} | ||
if math.Log2(float64(deckSize))*float64(handSize) > maxHashBits { | ||
return []string{fmt.Sprintf("more than %d bits of entropy required", maxHashBits)} | ||
} | ||
return | ||
} | ||
|
||
// ShuffleAndDeal can shuffle a hash value to handSize-quantity and non-redundant | ||
// indices of decks, with the pick function, we can get the optimal deck index | ||
// Eg. From deckSize=128, handSize=8, we can get an index array [12 14 73 18 119 51 117 26], | ||
// then pick function will choose the optimal index from these | ||
// Algorithm: https://github.com/kubernetes/enhancements/blob/master/keps/sig-api-machinery/20190228-priority-and-fairness.md#queue-assignment-proof-of-concept | ||
func ShuffleAndDeal(hashValue uint64, deckSize, handSize int32, pick func(int32)) { | ||
remainders := make([]int32, handSize) | ||
func ShuffleAndDeal(hashValue uint64, deckSize, handSize int, pick func(int)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @MikeSpreitzer @yue9944882 why do we change int32 to int here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I documented that in the commit comment. See it at aadd486 , for example. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I also wrote about it in another comment: #81182 (comment) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. got it, I think both are OK if we accept the type conversion in priority and fairness. cc @yue9944882 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. convinced, let's make it an int |
||
remainders := make([]int, handSize) | ||
|
||
for i := int32(0); i < handSize; i++ { | ||
for i := 0; i < handSize; i++ { | ||
hashValueNext := hashValue / uint64(deckSize-i) | ||
remainders[i] = int32(hashValue - uint64(deckSize-i)*hashValueNext) | ||
remainders[i] = int(hashValue - uint64(deckSize-i)*hashValueNext) | ||
hashValue = hashValueNext | ||
} | ||
|
||
for i := int32(0); i < handSize; i++ { | ||
for i := 0; i < handSize; i++ { | ||
candidate := remainders[i] | ||
for j := i; j > 0; j-- { | ||
if candidate >= remainders[j-1] { | ||
|
@@ -60,9 +74,9 @@ func ShuffleAndDeal(hashValue uint64, deckSize, handSize int32, pick func(int32) | |
} | ||
|
||
// ShuffleAndDealWithValidation will do validation before ShuffleAndDeal | ||
func ShuffleAndDealWithValidation(hashValue uint64, deckSize, handSize int32, pick func(int32)) error { | ||
if !ValidateParameters(deckSize, handSize) { | ||
return errors.New("bad parameters") | ||
func ShuffleAndDealWithValidation(hashValue uint64, deckSize, handSize int, pick func(int)) error { | ||
if errs := ValidateParameters(deckSize, handSize); len(errs) > 0 { | ||
return errors.New(strings.Join(errs, ";")) | ||
} | ||
|
||
ShuffleAndDeal(hashValue, deckSize, handSize, pick) | ||
|
@@ -71,14 +85,14 @@ func ShuffleAndDealWithValidation(hashValue uint64, deckSize, handSize int32, pi | |
|
||
// ShuffleAndDealToSlice will use specific pick function to return slices of indices | ||
// after ShuffleAndDeal | ||
func ShuffleAndDealToSlice(hashValue uint64, deckSize, handSize int32) []int32 { | ||
func ShuffleAndDealToSlice(hashValue uint64, deckSize, handSize int) []int { | ||
var ( | ||
candidates = make([]int32, handSize) | ||
candidates = make([]int, handSize) | ||
idx = 0 | ||
) | ||
|
||
pickToSlices := func(can int32) { | ||
candidates[idx] = can | ||
pickToSlices := func(can int) { | ||
candidates[idx] = int(can) | ||
idx++ | ||
} | ||
|
||
|
@@ -87,11 +101,33 @@ func ShuffleAndDealToSlice(hashValue uint64, deckSize, handSize int32) []int32 { | |
return candidates | ||
} | ||
|
||
// ShuffleAndDealIntoHand shuffles a deck of the given size by the | ||
// given hash value and deals cards into the given slice. The virtue | ||
// of this function compared to ShuffleAndDealToSlice is that the | ||
// caller provides the storage for the hand. | ||
func ShuffleAndDealIntoHand(hashValue uint64, deckSize int, hand []int) { | ||
handSize := len(hand) | ||
var idx int | ||
ShuffleAndDeal(hashValue, deckSize, handSize, func(card int) { | ||
hand[idx] = int(card) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. isn't idx always 0, since ShuffleAndDeal is called only once inside ShuffleAndDealIntoHand? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think Mike is right. |
||
idx++ | ||
}) | ||
} | ||
|
||
// ShuffleAndDealToSliceWithValidation will do validation before ShuffleAndDealToSlice | ||
func ShuffleAndDealToSliceWithValidation(hashValue uint64, deckSize, handSize int32) ([]int32, error) { | ||
if !ValidateParameters(deckSize, handSize) { | ||
return nil, errors.New("bad parameters") | ||
func ShuffleAndDealToSliceWithValidation(hashValue uint64, deckSize, handSize int) ([]int, error) { | ||
if errs := ValidateParameters(deckSize, handSize); len(errs) > 0 { | ||
return nil, errors.New(strings.Join(errs, ";")) | ||
} | ||
|
||
return ShuffleAndDealToSlice(hashValue, deckSize, handSize), nil | ||
} | ||
|
||
// ShuffleAndDealIntoHandWithValidation does validation and then ShuffleAndDealIntoHand | ||
func ShuffleAndDealIntoHandWithValidation(hashValue uint64, deckSize int, hand []int) error { | ||
if errs := ValidateParameters(deckSize, len(hand)); len(errs) > 0 { | ||
return errors.New(strings.Join(errs, ";")) | ||
} | ||
ShuffleAndDealIntoHand(hashValue, deckSize, hand) | ||
return nil | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we combine some if to make return in if?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could, but I chose to use the existing structure because it made a certain amount of sense. First, we check the range of each individual value, accumulating errors. If there are any then take an early out. Then check logically necessary relationships. Early out if that revealed problems. Finally do the more complicated and costly quantitative check. I thought that would be easiest on other peoples' minds. But since you objected, I will rewrite to be more direct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i agree with Mike, the current flow is easier to understand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yliaog : I am not sure which you are agreeing with. Is the revised code OK?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the current code structure looks good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK,let's keep this.