Call RPC getHealth during ingestion #77
Conversation
aditya1702
changed the title
| @@ -49,7 +56,7 @@ type Transaction struct { | |||
| ResultXDR string `json:"resultXdr"` | |||
| ResultMetaXDR string `json:"resultMetaXdr"` | |||
| Ledger int64 `json:"ledger"` | |||
| DiagnosticEventsXDR string `json:"diagnosticEventsXdr"` | |||
| DiagnosticEventsXDR []string `json:"diagnosticEventsXdr"` | |||
There was a problem hiding this comment.
why is this an array now?
There was a problem hiding this comment.
@gouthamp-stellar Ummm, this has always been an array since the time I added the GetTransactions endpoint - https://github.com/stellar/stellar-rpc/pull/174/files#diff-5276db8001730f32b267cab637b316958b69aef6f8184343338855fdc118b700R66
I was actually confused when I got a parsing error in wallet-backend when it was just a string. And also the fact that it has been working fine before too.
| case ingestLedger > resp.LatestLedger: | ||
| log.Debugf("waiting for RPC to catchup to ledger %d (latest: %d)", | ||
| ingestLedger, resp.LatestLedger) | ||
| time.Sleep(5 * time.Second) |
There was a problem hiding this comment.
Is this sleep of 5 seconds necessary? What happens if you remove it?
|
This mostly looks good, just one observation - once getHealth returns an actual healthy response, the expectation is that the ingestor should never get a "ledger out of range" type error when calling getTransactions. We just need to be sure that this implementation supports that |
| case resp := <-rpcHeartbeat: | ||
| switch { | ||
| // Case-1: wallet-backend is running behind rpc's oldest ledger. In this case, we start | ||
| // ingestion from rpc's oldest ledger. | ||
| case ingestLedger < resp.OldestLedger: | ||
| ingestLedger = resp.OldestLedger |
There was a problem hiding this comment.
Is this the desired functionality? Wouldn't a gap in the wallet backend's history be created if it was behind RPC's oldest ledger?
| err = m.ingestPayments(ctx, ledgerTransactions) | ||
| if err != nil { | ||
| return fmt.Errorf("error ingesting payments: %w", err) | ||
| } |
There was a problem hiding this comment.
I'm assuming ingestPayments is a function that ingests all inbound payments, since we have another function processTSSTransactions that I assume ingests all outbound payments? Maybe we should update the function names to make that more clear?
Actually, I'm assuming both functions commit changes to the DB, which means that its possible to process inbound payments for a ledger and fail to process the outbound payments. If thats true, wouldn't this result in a bad state where we wouldn't be able to reingest the ledger, since retries would fail at the inbound processing step?
Closes #62
This PR introduces the following: