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

cli: add retries if search results are not expected length in upload-bin #3650

Merged
merged 4 commits into from
Nov 2, 2024

Conversation

AliceInHunterland
Copy link
Contributor

@AliceInHunterland AliceInHunterland commented Oct 29, 2024

Close #3647

check empty oids in progress

@AliceInHunterland
Copy link
Contributor Author

@AnnaShaleva am I correct that we still need that flag for skipping?

Copy link

codecov bot commented Oct 29, 2024

Codecov Report

Attention: Patch coverage is 0.45249% with 220 lines in your changes missing coverage. Please review.

Please upload report for BASE (master@a4633ce). Learn more about missing BASE report.
Report is 5 commits behind head on master.

Files with missing lines Patch % Lines
cli/util/uploader.go 0.00% 220 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff            @@
##             master    #3650   +/-   ##
=========================================
  Coverage          ?   83.06%           
=========================================
  Files             ?      334           
  Lines             ?    46563           
  Branches          ?        0           
=========================================
  Hits              ?    38678           
  Misses            ?     6311           
  Partials          ?     1574           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@AnnaShaleva AnnaShaleva left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Waiting for the results of mainnet data consistency check, because the following error is caused by some other bug in the code:

failed to update index files after upload: empty oid found in index file 0 at position 91 (block index 91)

cli/util/uploader.go Outdated Show resolved Hide resolved
cli/util/uploader.go Outdated Show resolved Hide resolved
cli/util/uploader.go Outdated Show resolved Hide resolved
cli/util/convert.go Outdated Show resolved Hide resolved
docs/neofs-blockstorage.md Outdated Show resolved Hide resolved
cli/util/uploader.go Outdated Show resolved Hide resolved
@AnnaShaleva
Copy link
Member

AnnaShaleva commented Oct 29, 2024

OK, the solution with resulting OID batch size check is not working, we need to allow duplicating entries since containers may have an arbitrary number of blocks with the same height. But let's keep --skip-blocks-uploading flag.

Copy link
Member

@AnnaShaleva AnnaShaleva left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I need some more time to review the second commit.

cli/util/uploader.go Outdated Show resolved Hide resolved
@AnnaShaleva
Copy link
Member

@AliceInHunterland, review the updated code. Make sure you understand routines-related code. Leave comments if something is not clear or invalid, we'll discuss it then. Move unrelated style changes to separate commits. Check if this version works properly on mainnet.

cli/util/uploader.go Outdated Show resolved Hide resolved
cli/util/uploader.go Outdated Show resolved Hide resolved
cli/util/uploader.go Show resolved Hide resolved
cli/util/uploader.go Outdated Show resolved Hide resolved
cli/util/uploader.go Outdated Show resolved Hide resolved
cli/util/uploader.go Outdated Show resolved Hide resolved
Copy link
Member

@AnnaShaleva AnnaShaleva left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some style changes are still leaking from the second commit to the third.

Copy link
Member

@AnnaShaleva AnnaShaleva left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise LGTM. Try it on mainnet and see if at least a single index file may be uploaded.

cli/util/uploader.go Outdated Show resolved Hide resolved
cli/util/uploader.go Outdated Show resolved Hide resolved
In case of incomplete search result it will try to find each missed oid
and process it. In case of duplicates the first found will be in index
file.

Close #3647

Signed-off-by: Ekaterina Pavlova <[email protected]>
Copy link
Member

@AnnaShaleva AnnaShaleva left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, waiting for the fix of nspcc-dev/neofs-node#2988 to test it on NeoFS mainnet.

@AnnaShaleva AnnaShaleva added the blocked Can't be done because of something label Nov 1, 2024
@AliceInHunterland
Copy link
Contributor Author

@AnnaShaleva as we can get oid when putting we can do something like this a0a0358?

@AnnaShaleva
Copy link
Member

as we can get oid when putting we can do something like this a0a0358?

It's a separate optimisation, not related to #3647. Create an issue for that, add reference to commit. And uploadIndexFiles can't be removed anyway, we must consider cases when you don't have all OIDs in-place.

@AnnaShaleva
Copy link
Member

And for the current PR in a separate commit replace HEAD with GET to retrieve block index by OID. We'll revert it once nspcc-dev/neofs-node#2988 is fixed.

Will be reverted when nspcc-dev/neofs-node#2988 is fixed. Currently HEAD
request can return incorrect attributes, which can lead to incorrect
index files.

Signed-off-by: Ekaterina Pavlova <[email protected]>
@AnnaShaleva AnnaShaleva removed the blocked Can't be done because of something label Nov 2, 2024
@AnnaShaleva AnnaShaleva merged commit 21d6887 into master Nov 2, 2024
35 checks passed
@AnnaShaleva AnnaShaleva deleted the index-files branch November 2, 2024 09:28
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.

upload-bin hangs on index files construction if at least one SEARCH result is incomplete
2 participants