-
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
Cache storage policy application results (Get
/Head
/GetRange
)
#2896
Conversation
19faa8a
to
383e341
Compare
Get
)Get
/Head
/GetRange
)
383e341
to
242212e
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2896 +/- ##
==========================================
+ Coverage 23.73% 23.78% +0.05%
==========================================
Files 775 775
Lines 44933 44970 +37
==========================================
+ Hits 10664 10698 +34
Misses 33417 33417
- Partials 852 855 +3 ☔ View full report in Codecov by Sentry. |
d24fb33
to
380a6b4
Compare
var j, jLim uint | ||
primary := true | ||
|
||
for i := 0; i < len(nodeLists); i++ { // do not use for-range! |
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? the comment does not explain, so it is useless (or even harmful)
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.
usually range is used for this, but here it wouldn't work
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.
only after reading it twice (to the end), i saw i = -1
, why not mention it? it is a comment, it should help. any code can be changed, so comments should explain something, and if they are outdated, it should be easy to understand. not saying it is required here but it is a general rule i would say
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.
Tricky one. Can be simplified by inverting i/j loops: walk over the top ones from each list first and then go deeper till there are no more nodes to check. It can be more optimal in some cases (top nodes from different lists are still more likely to have the object), but maybe not for all of them (policies may have local nodes first).
The other way of course is just for _, primary := range []bool{true, false}
and a regular range loop over nodeLists
then. Less tricks, same meaning.
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.
top nodes from different lists are still more likely to have the object
they are homogeneous in terms of policies
for _, primary := range []bool{true, false}
i can make any style you think is readable. I'm fine with current. It can be simply divided into two loops. No tricks at all
@@ -36,18 +48,24 @@ type containerNodes struct { | |||
containers container.Source | |||
network netmap.Source | |||
|
|||
cache *lru.Cache[containerNodesCacheKey, storagePolicyRes] | |||
cache *lru.Cache[containerNodesCacheKey, storagePolicyRes] |
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.
rename too?
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.
no need, it contains container nodes
Some conflicts. |
Continues 2f29338 for `ObjectService`'s `Get`/`Head`/`GetRange` server handlers. Signed-off-by: Leonard Lyubich <[email protected]>
380a6b4
to
c598f34
Compare
Continues 10d05a4 for sorting container nodes for objects. Since each container may include plenty of objects, cache size limit is chosen - again heuristically - 10 times bigger, i.e. 10K. Refs #2692. Signed-off-by: Leonard Lyubich <[email protected]>
They are vendored by NeoFS SDK, but the reaction to their unexpected behavior also needs to be tested. Signed-off-by: Leonard Lyubich <[email protected]>
c598f34
to
8b82bbd
Compare
What's the status here? Some threads are not fully answered and no review request yet. |
i've already processed all of them, pls ref me to the exact ones |
Search
,Replicate
) #2892i created 2 containers with 5 object in each and ran 30 routines sending 1000 GET queries (each time random object)
v0.42.1
current branch
Go script