-
Notifications
You must be signed in to change notification settings - Fork 38
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
upd/HRW library #2629
upd/HRW library #2629
Conversation
@@ -96,11 +96,6 @@ func (e *StorageEngine) Evacuate(prm EvacuateShardPrm) (EvacuateShardRes, error) | |||
} | |||
e.mtx.RUnlock() | |||
|
|||
weights := make([]float64, 0, len(shards)) | |||
for i := range shards { | |||
weights = append(weights, e.shardWeight(shards[i].Shard)) |
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.
What if we have different capacity for every shard?
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 can add weight to sort it more "honestly" and will not know what weight to use when a Get
request is here
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 better keep the functionality as is then and create an issue to solve it in the future. We will need this, shard can be put into/retrieved from the metabase.
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.
shard can be put into/retrieved from the metabase.
if meta is not per shard (not the current case)
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.
Sure, sure. But the weighting itself should be kept.
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.
But the weighting itself should be kept.
you mean this PR? this code is currently broken for sure: it always uses 0
and confuses. i suggest returning weighting (reworking in fact, the current version can't be used at all) when we need it and when we are ready for it
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.
Why do you think it needs a complete rework?
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.
- it is not used and not usable. i mean totally. it is commented as
Amount of free disk space. Measured in kilobytes.
and this field is never changed (always zero). and never was changed from the beginnings - shard currently does not know how much it stores. just no code uses it, nobody ever asked about it. some estimation metrics per container, some calculations inside bbolt's transactions, some internal changes by background workers, and so on. all of it does not use this field (see
1.
) - if we want it to be used as a weight, it will be the first storage use case, so meta per shard should be reworked
i do not mean weighting is hard to implement, but some things should be done first, and currently it just does wrong things
088f469
to
3c49f54
Compare
Signed-off-by: Pavel Karpy <[email protected]>
Every shard has a zero weight. It is useless and worked only because of the falling back to the simple sorting in the HRW library. Signed-off-by: Pavel Karpy <[email protected]>
3c49f54
to
a1cbaac
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #2629 +/- ##
=======================================
Coverage 28.67% 28.67%
=======================================
Files 415 414 -1
Lines 32234 32251 +17
=======================================
+ Hits 9244 9249 +5
- Misses 22195 22205 +10
- Partials 795 797 +2 ☔ View full report in Codecov by Sentry. |
No description provided.