Skip to content
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

fix/Estimations announcement #2597

Merged
merged 3 commits into from
Sep 29, 2023

Conversation

carpawell
Copy link
Member

No description provided.

@codecov
Copy link

codecov bot commented Sep 27, 2023

Codecov Report

Merging #2597 (5a0006b) into master (1968c36) will increase coverage by 0.00%.
The diff coverage is 46.15%.

❗ Current head 5a0006b differs from pull request most recent head 3e4e712. Consider uploading reports for the commit 3e4e712 to get more accurate results

@@           Coverage Diff           @@
##           master    #2597   +/-   ##
=======================================
  Coverage   29.71%   29.71%           
=======================================
  Files         407      407           
  Lines       31143    31145    +2     
=======================================
+ Hits         9254     9255    +1     
  Misses      21078    21078           
- Partials      811      812    +1     
Files Coverage Δ
...es/container/announcement/load/controller/calls.go 65.35% <75.00%> (-0.65%) ⬇️
pkg/local_object_storage/engine/container.go 0.00% <0.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@carpawell
Copy link
Member Author

Seems like we also need a routine that is going to iterate over containers and be ready to clean up every info on every container removal (also, the storage engine should support this operation too, of course). But it is a too big change so not added it yet to this PR. We have some removal subscription but it won't work for the removals at the SN's offline time.

Thoughts? @roman-khimov, @cthulhu-rider, @AliceInHunterland

@carpawell carpawell force-pushed the fix/estimations-announcement branch 2 times, most recently from afa9110 to 053137b Compare September 27, 2023 16:10
@roman-khimov
Copy link
Member

clean up every info on every container removal

Are you talking about #1663 or some kind of locally stored metabase thing related to estimations specifically?

@carpawell
Copy link
Member Author

carpawell commented Sep 27, 2023

some kind of locally stored metabase thing related to estimations specifically?

Like this, yes. If a node is sleeping it won't be able to drop its indexes with the notifications approach (that we already have). But yes, now it does absolutely nothing if we are talking about the estimations but I do not want to fix it fast cause notifications is only a part of potentially useless info in the metabase if a container is dropped, so want to hear your thoughts and maybe create another issue for more accurate solution.

But the current missing container issue is fixed with this PR, although I would not treat it as the best solution for the exact that problem.

@roman-khimov
Copy link
Member

I'd forget this problem for now, estimation mechanism itself won't be like this forever.

CHANGELOG.md Outdated Show resolved Hide resolved
@roman-khimov
Copy link
Member

Needs a rebase, CHANGELOG has changed.

Iterations over the locally calculated values should not stop because of any
errors. Sending part of the information is better than sending nothing in this
case. Moreover, the `code = 3072 message = container not found` logs were found,
they are definitely not a good reasons to stop the iterations over the remaining
estimations.
Refs nspcc-dev#1506.

Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>
In particular, it would save from estimating removed containers found in the
logs (they were empty), but it is also useless in general.
Refs nspcc-dev#1506.

Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>
`ListContainers` do not use a string container ID representation so do not do
that useless conversion.

Signed-off-by: Pavel Karpy <carpawell@nspcc.ru>
@roman-khimov roman-khimov merged commit 413a6d2 into nspcc-dev:master Sep 29, 2023
7 of 8 checks passed
@carpawell carpawell deleted the fix/estimations-announcement branch September 29, 2023 12:32
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.

None yet

3 participants