Make fog-view load blocks from postgres in batches #2707
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
These batches have a size which is configurable as a command line parameter.
cbeck88
commented
Oct 12, 2022
| // We might have more work to do, we aren't sure because of the error | ||
| may_have_more_work = true; | ||
| // Let's back off for one interval when there is an error | ||
| sleep(DB_POLL_INTERNAL); |
There was a problem hiding this comment.
I am not sure if this is a good idea, but right now I think nothing backs off if we hit the error path, which might be a bad thing.
This is not directly related to the main change.
cbeck88
commented
Oct 12, 2022
| let proto = ProtoIngestedBlockData::decode(&*proto)?; | ||
| result.push(proto.e_tx_out_records); | ||
| } else { | ||
| log::warn!(self.logger, "When querying for block index {} and up to {} blocks on, the {}'th response has block_number {} which is not expected. Gaps in the data?", block_index, block_count, idx, block_number); |
There was a problem hiding this comment.
there is not much reason to think that this code path will happen, but I would rather be defensive
eranrund
requested changes
Oct 12, 2022
remoun
reviewed
Oct 12, 2022
|
this passed CD |
eranrund
approved these changes
Oct 13, 2022
remoun
approved these changes
Oct 13, 2022
| }; | ||
|
|
||
| // We will get one row for each hit in the table we found | ||
| let rows: Vec<(i64, Vec<u8>)> = query.load::<(i64, Vec<u8>)>(&conn)?; |
There was a problem hiding this comment.
IIUC we don't need to specify the type twice
Suggested change
| let rows: Vec<(i64, Vec<u8>)> = query.load::<(i64, Vec<u8>)>(&conn)?; | |
| let rows: Vec<(i64, Vec<u8>)> = query.load(&conn)?; |
| let db_test_context = SqlRecoveryDbTestContext::new(logger.clone()); | ||
| let db = db_test_context.get_db_instance(); | ||
| let db_fetcher = DbFetcher::new(db.clone(), Default::default(), logger); | ||
| let db_fetcher = DbFetcher::new(db.clone(), Default::default(), 1, logger); |
There was a problem hiding this comment.
It seems useful to exercise a DbFetcher with batch_size > 1 in this file
There was a problem hiding this comment.
yeah, we do that in the smoke test, but I thought it's okay to keep this unit test working with 1
wjuan-mob
reviewed
Oct 13, 2022
| /// Arguments: | ||
| /// * ingress_key: The ingress key we need ETxOutRecords from | ||
| /// * block_index: The first block we need ETxOutRecords from | ||
| /// * block_count: How many subsequent blocks to also request data for. |
There was a problem hiding this comment.
Maybe this should be how many consecutive blocks to also request data for? Subsequent seems to suggest 0 should just return only this block_index, and 1 means return this and 1 additional?
| /// Arguments: | ||
| /// * ingress_key: The ingress key we need ETxOutRecords from | ||
| /// * block_index: The first block we need ETxOutRecords from | ||
| /// * block_count: How many subsequent blocks to also request data for. |
There was a problem hiding this comment.
Similar comment to above.
Co-authored-by: Remoun Metyas <remoun@mobilecoin.com>
cbeck88
added a commit
that referenced
this pull request
Oct 14, 2022
* Make fog-view load blocks from postgres in batches These batches have a size which is configurable as a command line parameter. * fixup tests * add unit test for new function * address eran and remoun comments in db_fetcher * change parameter to a usize * rename per request * Update fog/sql_recovery_db/src/lib.rs Co-authored-by: Remoun Metyas <remoun@mobilecoin.com> Co-authored-by: Remoun Metyas <remoun@mobilecoin.com>
cbeck88
added a commit
that referenced
this pull request
Oct 17, 2022
* Make fog-view load blocks from postgres in batches (#2707) * Make fog-view load blocks from postgres in batches These batches have a size which is configurable as a command line parameter. * fixup tests * add unit test for new function * address eran and remoun comments in db_fetcher * change parameter to a usize * rename per request * Update fog/sql_recovery_db/src/lib.rs Co-authored-by: Remoun Metyas <remoun@mobilecoin.com> Co-authored-by: Remoun Metyas <remoun@mobilecoin.com> * Nits from fog view db fetch pr (#2714) * fix nits from fog-view-db-fetch pull request * update changelog Co-authored-by: Remoun Metyas <remoun@mobilecoin.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
These batches have a size which is configurable as a command line parameter.
The point of this is that we have been having issues where the network latency to get from fog-view to postgres has been high enough that it becomes the bottleneck for fog-view loading all the data into ORAM, and begins to significantly negatively impact the time to restart fog view. We can make the server more tolerant of that by making it try to load more data at once.
Thanks to James for suggested approach here