-
Notifications
You must be signed in to change notification settings - Fork 90
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
core/priority: refactor priority protocol #1271
Conversation
for _, topic := range msg.Topics { | ||
if dedupTopics(msg.PeerId) { |
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.
bug!
Codecov ReportBase: 53.75% // Head: 53.74% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #1271 +/- ##
==========================================
- Coverage 53.75% 53.74% -0.01%
==========================================
Files 139 139
Lines 16365 16446 +81
==========================================
+ Hits 8797 8839 +42
- Misses 6312 6345 +33
- Partials 1256 1262 +6
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
// Calculate overall score for all priorities in the topic | ||
// which effectively orders by count then by overall priority. | ||
var ( | ||
scores = make(map[string]int) | ||
allPriorities []string | ||
scores = make(map[[32]byte]int) |
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.
can we give [32]byte
a type? will make it more readable
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.
yeah not sure, I actually removed the instanceKey
type, reverting to just [32]byte
. I think defining types is mostly useful if the scope of the type is very large. In this case, it is pretty small.
core/priority/prioritiser.go
Outdated
// Priority is one of many grouped by a Topic being prioritised in an Instance. | ||
type Priority proto.Message | ||
|
||
// instanceKey is the hash of an Instance proto.Message and is used to index instanceData. See hashProto. |
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.
// instanceKey is the hash of an Instance proto.Message and is used to index instanceData. See hashProto. | |
// instanceKey is the hash of an Instance and is used to index instanceData. See hashProto. |
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.
think the original is fine
Refactors the priority protocol:
anypb.Any
to support arbitrary use-cases.category: refactor
ticket: #1257