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

Storage Server Version Triage #1007

Merged
merged 17 commits into from
Mar 27, 2020
Merged

Storage Server Version Triage #1007

merged 17 commits into from
Mar 27, 2020

Conversation

neuroscr
Copy link

@neuroscr neuroscr commented Mar 25, 2020

  • builds a pool per version (and a reverse map)
  • filters pools with 2.0.2 or greater for file server proxy
  • mark as bad removes from random and version pools
  • increased start up retries from 1s to 10s (as this PR adds 10-20s delay to proxy services while we get versions and sorts in the usable buckets)
  • takes about 10mins to get versions from 980 sndoes

@neuroscr neuroscr requested review from msgmaxim and Mikunj and removed request for msgmaxim March 25, 2020 09:35
@Mikunj
Copy link

Mikunj commented Mar 25, 2020

increased start up retries from 1s to 10s (as this PR adds 10-20s delay to proxy services while we get versions and sorts in the usable buckets)

What would this mean in terms of startup time and receiving messages?

takes about 10mins to get versions from 980 snodes

Wouldn't it be better to query snodes until we have X amount that are of a certain version, and keep doing it in the background?

@neuroscr
Copy link
Author

neuroscr commented Mar 25, 2020

What would this mean in terms of startup time and receiving messages?

Only should really affect proxy requests (RSS, version check, attachments, device mapping fetching)

Wouldn't it be better to query snodes until we have X amount that are of a certain version, and keep doing it in the background?

The version fetching is backgrounded for the most part.

@neuroscr
Copy link
Author

Notes for V2:

  • sql storage
  • store version in random pool and just filter it

Copy link

@Mikunj Mikunj left a comment

Choose a reason for hiding this comment

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

Looks good for v1, will wait on what Maxim has to say and we can decide is we want to merge v1 and v2 seperate or just have all the changes in together

@neuroscr
Copy link
Author

Refined patch to only affect file upload (attachments/avatars) proxy requests allowing RSS, version check, and device mapping fetching to proceed normally since their content is usually under 1mb.

Also handled some edge cases in the markRandomSNodeBad when we haven't got it's version yet, so we retry, so we can mark it bad if/when we do get it.

await this.refreshRandomPool();
}
if (this.randomSnodePool.length === 0) {
throw new window.textsecure.SeedNodeError('Invalid seed node response');

Choose a reason for hiding this comment

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

Should we throw this in refreshRandomPool() instead?

Copy link
Author

Choose a reason for hiding this comment

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

Not sure, @Mikunj do you know if this is an issue that we throw inside a promise instead of the outer function?

Choose a reason for hiding this comment

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

Throwing inside async should reject the promise and awaiting on rejected promise should just re-throw the original exception.

Copy link
Author

Choose a reason for hiding this comment

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

which we catch on line 483 and rethrow

@neuroscr
Copy link
Author

I really hate how this floods the log with no version retries but we can fix up that later

@neuroscr neuroscr merged commit 7412b2d into oxen-io:clearnet Mar 27, 2020
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