-
Notifications
You must be signed in to change notification settings - Fork 499
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
historyarchive: Introduce a History Archive pool that's selected from for all calls #3402
Conversation
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.
LGTM! I agree with @tamirms's type PooledArchive []ArchiveInterface
idea, it should be easier to use from outside.
historyarchive/pool.go
Outdated
// Copyright 2016 Stellar Development Foundation and contributors. Licensed | ||
// under the Apache License, Version 2.0. See the COPYING file at the root | ||
// of this distribution or at http://www.apache.org/licenses/LICENSE-2.0 | ||
|
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.
I don't think we need copyright comment on top of this file. At least we don't have it in most of other files. @ire-and-curses?
// Copyright 2016 Stellar Development Foundation and contributors. Licensed | |
// under the Apache License, Version 2.0. See the COPYING file at the root | |
// of this distribution or at http://www.apache.org/licenses/LICENSE-2.0 |
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.
Re: copyright - let's just follow whatever we do everywhere else in the codebase.
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.
Yeah so I was confused by this, too. In the historyarchive
package, all of the files have copyright notices. The years are mixed, but all of them have one. So I added it here.
I don't understand Golang *super* well yet, but I think that methods on a *Type are way less restrictive than methods on a Type, right?
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.
looks great! I left just one minor comment about the constructor name
PR Checklist
PR Structure
otherwise).
services/friendbot
, orall
ordoc
if the changes are broad or impact manypackages.
Thoroughness
.md
files, etc... affected by this change). Take a look in the
docs
folder for a given service,like this one.
Release planning
needed with deprecations, added features, breaking changes, and DB schema changes.
semver, or if it's mainly a patch change. The PR is targeted at the next
release branch if it's not a patch change.
What
This is an improvement over #3375 which just chooses a random URL from the list. Instead, we should actually create a pool of all history archive connections and cycle through them randomly for each HA-related operation we need.
Why
With this change, we actually use the entire URL list in the configuration that the user provides. This adds resilience and redundancy. See stellar/horizon-internal#3 for more context.
Known limitations
n/a