-
Notifications
You must be signed in to change notification settings - Fork 135
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
Partner Experience: update horizon config/install copy #162
Partner Experience: update horizon config/install copy #162
Conversation
…nt, changed 'api server' to 'platform service'
Preview is available here: |
Preview is available here: |
Preview is available here: |
1 similar comment
Preview is available here: |
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 did a first-pass review!
Lots of minor comments oriented around phrasing and "proper" documentation. Imo, as a general rule, official docs should be as easy to copy-paste as possible, so I tried to make suggestions oriented around e.g. maintaining complete sentences, expanding abbreviations, phrasing changes for clarity, etc.
There are also a handful of grammatical fixes. We should definitely get devrel (and maybe even someone from marketing) do a proper wording + grammar pass before merging this branch into main
.
yes, these changes won't be landing on |
Co-authored-by: George <[email protected]>
Co-authored-by: George <[email protected]>
Preview is available here: |
1 similar comment
Preview is available here: |
Co-authored-by: George <[email protected]>
Preview is available here: |
Co-authored-by: George <[email protected]>
Preview is available here: |
Preview is available here: |
Preview is available here: |
Preview is available here: |
- for any other hosts, download [prebuilt release binaries](#prebuilt-releases) of Stellar Horizon and Core for host target architecture and operation system. | ||
- containerized: | ||
- non-Helm Chart, if the target envrironment for container to run does not support Helm chart usage, run the prebuilt docker image of Horizon published on [dockerhub.com/stellar/horizon](https://hub.docker.com/r/stellar/stellar-horizon). | ||
- Helm charts, when the target envrionment uses container orchestration such as Kubernetes and has enabled Helm Charts on cluster. The Horizon Helm chart manages installation life cycle. Use the [Helm Install command](https://helm.sh/docs/helm/helm_install/), it will accept Horizon's configuration parameters. Please review [Configuration](./configuring.mdx) first, to identify any specific configuration params needed. **TODO - create the Horizon Helm Chart.** |
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.
Hello @jacekn , as part of new documentation, we are referring to the potential Helm Chart usage for Horizon. Wondering if we shouild spin up a dev ticket for Horizon Helm Chart on the mono repo, or if you are aware of Helm for Horizon already, should we coordinate with an Ops ticket for this feature? CC'ing @urvisavla
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.
ok to merge w/ TODOs? if yes I can ✔️ but @mollykarcher / Jesse (what's his GH username??) should look as well for product/partner perspective
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've added a few minor comments but other than that it looks great!
Co-authored-by: George <[email protected]>
Preview is available here: |
Preview is available here: |
Preview is available here: |
yes, we are spinning up dev tickets for the TODO's, and will remove the TODO's from here first before merge. |
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.
Took a first pass, overall I think this is a great start!
I started to add a couple of these, but I decided to forgo any/all small formatting/grammar/nit-picking type of comments. I think we should probably hold these kinds of things until a final pass before we merge it all back into main. There'll just be too much noise with all the movement/changes if we attempt that before then.
|
||
## Multiple Instance Deployment | ||
|
||
You run multiple instances of `stellar-horizon` each performs a subset of the roles. |
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.
You run multiple instances of `stellar-horizon` each performs a subset of the roles. | |
In this scalable deployment variant, you run multiple instances of `stellar-horizon` and each performs a subset of the roles. This allows you to horizontally scale each of the functions independently. |
Yep 👍 aligned with this approach |
Co-authored-by: Molly Karcher <[email protected]>
Preview is available here: |
Preview is available here: |
Preview is available here: |
Preview is available here: |
Preview is available here: |
Preview is available here: |
|
||
- `NETWORK`, optional, can be one of Stellar's public networks, 'pubnet', or 'devnet'. Triggers horizon to automatically set configurations for remaining horizon settings and generate the correct core toml/cfg settings. If you only need Horizon to connect to one of those public Stellar networks, this will take care of all related configuration. | ||
|
||
**TODO - create the new `NETWORK` env variable and the functionality it performs to configure all aspects of network connection, including toml, passphrase, archive urls, etc for networks 'pubnet' and 'testnet'. Refer to quickstart for example of same consolidated config parameter for setting network. ** |
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'm creating a dev ticket for this task and wanted to clarify a few points
-
If "NETWORK" is not specified, Horizon will still automatically set the values for CAPTIVE_CORE_CONFIG_PATH and CAPTIVE_CORE_STORAGE_PATH.
-
The CAPTIVE_CORE_STORAGE_PATH is not dependent on the network chosen. So the default storage path will remain the same regardless of the network.
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.
If "NETWORK" is not specified, Horizon will still automatically set the values for CAPTIVE_CORE_CONFIG_PATH and CAPTIVE_CORE_STORAGE_PATH.
Do you see that behavior based on NETWORK-PASSPHRASE
, I found an auto network config feature done in past based on that value, go#3783, was not aware of this! I think the intent for its usage was more oriented to ease for dev, the design discussion struggled with prod usage, which we should learn from.
@mollykarcher , I wonder how much this auto-config default behavior of NETWORK-PASSPHRASE
is being relied upon in the field? It's not mentioned in the -help output, seems kind of quietly in there. Should we focus on shoring up the existing auto-config functionality of NETWORK-PASSPHRASE
and referring to that usage here in docs or move forward with NETWORK
? I kinda of prefer the latter as the former seems to conflate the meaning of what appears to be just a value-based setting, but need a check on this.
In either case for auto-config, I don't think Horizon should bother to configure default network or captive core settings if the key network settings are absent, as there is a valid configuration for the API role, where INGEST=false, TX_SUB_ENABLED=false and no network access is required, only the database is used.
@urvisavla , this touches on need for new validations on config param settings that we should capture in the ticket also which we propose Horizon to detect/warn about, i.e. if INGEST=true or TX_SUB_ENABLED=true then network config settings must be present, either the key auto-config parameter or the collection of lower level params.
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.
Oooh nice find on that ticket. I would agree with you in that, I prefer NETWORK
, it feels cleaner and more intentional than the alternative.
Preview is available here: |
You are now ready to setup configuration parameters to perform three important, distinct roles: | ||
|
||
- **serving requests** via a regular web-based HTTP API, and | ||
- **ingesting ledgers** from core nodes of a Stellar network to keep its world-view up to date, and |
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.
@mollykarcher , @urvisavla , @Shaptic , wondering about a possible fourth role..a 'current state' role, a horizon instance that runs ingestion w/captive core, but stores no history, just the current state. provides cohort of api endpoints that come with that data only such as order book, offers, trades(trade-agg?), etc, not sure if enough app uses cases fall into this.
the role would equate to lighter-weight deployment, likely not external postgres/db, maybe get by with transient sqlite db spun up internally on disk/mem, this may have been evaluated already.
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.
a horizon instance that runs ingestion w/captive core, but stores no history, just the current state.
Would this be same as running ingestion with no history retention? I just want to clarify my understanding of running ingestion with no history.
The main goal of docs updates in terms of partner experience is to make them more opinionated on what to do for configuration with less choices so it is faster for setup and less possibility of choosing wrong settings. The effort here was to remove many options and instead present as few but generally robust options that are geared towards latest settings in horizon.
This change is focused on just runtime configuration(not ingestion yet, that is another task).
Configuring
was refactored and I pulled in reformat ofInstalling
as it was closely tied into how configurations get done as well. You can view new pages here:https://developers-pr162.previews.kube001.services.stellar-ops.com/docs/run-platform-server/installing
https://developers-pr162.previews.kube001.services.stellar-ops.com/docs/run-platform-server/configuring
To that means, have also made leap in docs to suggest features/flags in Horizon to enable these streamlined configs, but don't exist yet, and therefore would need to be reviewed to determine if we want to add them. I've highlighted any functional gaps with TODO inline in docs to mark where these are.
Closes #141