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

exp/services/captivecore/internal: Add HTTP server for managing a remote captive core instance #2896

Merged
merged 5 commits into from
Aug 11, 2020

Conversation

tamirms
Copy link
Contributor

@tamirms tamirms commented Aug 6, 2020

PR Checklist

PR Structure

  • This PR has reasonably narrow scope (if not, break it down into smaller PRs).
  • This PR avoids mixing refactoring changes with feature changes (split into two PRs
    otherwise).
  • This PR's title starts with name of package that is most changed in the PR, ex.
    services/friendbot, or all or doc if the changes are broad or impact many
    packages.

Thoroughness

  • This PR adds tests for the most critical parts of the new functionality or fixes.
  • I've updated any docs (developer docs, .md
    files, etc... affected by this change). Take a look in the docs folder for a given service,
    like this one.

Release planning

  • I've updated the relevant CHANGELOG (here for Horizon) if
    needed with deprecations, added features, breaking changes, and DB schema changes.
  • I've decided if this PR requires a new major/minor version according to
    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

Add HTTP server for managing a remote captive core instance.

Why

From #2841 :

The current implementation of Captive Stellar-Core streams meta stream using a filesystem pipe. This implies that both Horizon and Stellar-Core have to be deployed to the same server. One of the disadvantages of such requirement is a need for detailed per-process monitoring to be able to connect potential issues (like memory leaks) to the specific service.

To solve this we can build a thin wrapper around CaptiveStellarCore backend that exposes a simple RPC/HTTP API similar to LedgerBackend interface:

 // LedgerBackend represents the interface to a ledger data store. 
 type LedgerBackend interface { 
 	// GetLatestLedgerSequence returns the sequence of the latest ledger available 
 	// in the backend. 
 	GetLatestLedgerSequence() (sequence uint32, err error) 
 	// The first returned value is false when the ledger does not exist in a backend. 
 	GetLedger(sequence uint32) (bool, xdr.LedgerCloseMeta, error) 
 	// PrepareRange prepares the given range (including from and to) to be loaded. 
 	// Some backends (like captive stellar-core) need to initalize data to be 
 	// able to stream ledgers. 
 	PrepareRange(ledgerRange Range) error 
 	// IsPrepared returns true if a given ledgerRange is prepared. 
 	IsPrepared(ledgerRange Range) bool 
 	Close() error 
 } 

The full solution requires a server and a client that can consume the API.

A server can be a subcommand on horizon that exposes the CaptiveStellarCore object API using either the net/rpc package or HTTP. It has the same require params as CaptiveStellarCore. A server is a singleton, meaning it can start a single stellar-core process, thus should be used by a single Horizon instance.

A client, RemoteCaptiveStellarCore implementation of LedgerBackend interface is part of exp/ingest/ledgerbackend and accepts a connection params to a server.

Known limitations

I will add tests in a subsequent commit. Right now I'm looking for a preliminary review

@tamirms tamirms requested a review from bartekn August 6, 2020 18:12
@cla-bot cla-bot bot added the cla: yes label Aug 6, 2020

mux.Post("/prepare-range", func(w http.ResponseWriter, r *http.Request) {
ledgerRange := ledgerbackend.Range{}
if err := json.NewDecoder(r.Body).Decode(&ledgerRange); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: I think form urlencoded would be much simpler here. JSON is fine but we don't really need it here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's slightly easier for the client code to use JSON because serializing to JSON would require less code than constructing an HTTP form. Also, it is nice to make the content type consistent between the request and response bodies.

// LedgerResponse is the response for the GetLedger command.
type LedgerResponse struct {
Present bool `json:"present"`
Ledger xdr.LedgerCloseMeta `json:"ledger"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it render as base64? If not the value will be really hard to parse.

@tamirms tamirms requested a review from a team August 11, 2020 09:49
@tamirms
Copy link
Contributor Author

tamirms commented Aug 11, 2020

@bartekn can you take another look? I've added tests now

@tamirms tamirms merged commit 742703c into stellar:master Aug 11, 2020
@tamirms tamirms deleted the captive-core-server branch August 11, 2020 16:04
tamirms added a commit that referenced this pull request Aug 19, 2020
…lient (#2918)

This PR is a follow up to #2896 . In #2896 we added an http server wrapper around stellar core so that we could run a captive core instance on a separate machine from horizon.

This PR adds the corresponding HTTP client for the captive core server. The client satisfies the LedgerBackend interface and can be plugged into Horizon's ingestion system.
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.

2 participants