Skip to content

Fix storage node syncing query#4621

Merged
mnaamani merged 4 commits intoJoystream:masterfrom
Lezek123:fix-colossus-sync
Feb 10, 2023
Merged

Fix storage node syncing query#4621
mnaamani merged 4 commits intoJoystream:masterfrom
Lezek123:fix-colossus-sync

Conversation

@Lezek123
Copy link
Copy Markdown
Contributor

@Lezek123 Lezek123 commented Feb 9, 2023

Addresses #4615 by splitting getAllAssignedDataObjects query into smaller queries, so that no individual query has more than 1000 storage bag ids provided in the input (to avoid hitting the 100kb request size limit)

@vercel
Copy link
Copy Markdown

vercel bot commented Feb 9, 2023 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Updated
pioneer-testnet ⬜️ Ignored (Inspect) Feb 10, 2023 at 8:15AM (UTC)

mnaamani
mnaamani approved these changes Feb 9, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@mnaamani mnaamani left a comment

Choose a reason for hiding this comment

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

Fix looks correct, serially making multiple queries is fine. Would doing multiple queries in parallel give a faster response?

zeeshanakram3
zeeshanakram3 reviewed Feb 10, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@zeeshanakram3 zeeshanakram3 left a comment

Choose a reason for hiding this comment

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

Thanks for the fast fix!

  1. I think we should add the onError error handling Apollo link while initializing the Query-node apollo client in storage node, similar to how it's implemented in the distributor-node's QueryNodeApi. This will imporve error handling/logging for graphql syntax & network errors
  2. Should not be in the scope of this PR I think, but we should analyze all the queries having structure similar to storageDataObjectsConnection that are being used in all of the services/nodes, i.e. queries having one or more array params. And then if there is a chance that those queries could too break the default request size limit, we should split those queries too into smaller ones

@Lezek123 Lezek123 mentioned this pull request Feb 10, 2023
Open
@Lezek123
Copy link
Copy Markdown
Contributor Author

Lezek123 commented Feb 10, 2023

think we should add the onError error handling Apollo link while initializing the Query-node apollo client in storage node, similar to how it's implemented in the distributor-node's QueryNodeApi. This will imporve error handling/logging for graphql syntax & network errors

Added in fa7cfb3

Should not be in the scope of this PR I think, but we should analyze all the queries having structure similar to storageDataObjectsConnection that are being used in all of the services/nodes, i.e. queries having one or more array params. And then if there is a chance that those queries could too break the default request size limit, we should split those queries too into smaller ones

Created a new issue: #4624

zeeshanakram3
zeeshanakram3 approved these changes Feb 10, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@zeeshanakram3 zeeshanakram3 left a comment

Choose a reason for hiding this comment

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

LGTM

@mnaamani mnaamani merged commit c775b83 into Joystream:master Feb 10, 2023
@Lezek123 Lezek123 mentioned this pull request Feb 21, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mnaamani mnaamani mnaamani approved these changes

@zeeshanakram3 zeeshanakram3 zeeshanakram3 approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Lezek123 @mnaamani @zeeshanakram3