Fixing the current CI failures.
The tests objective is to exercise the chain synchronization code, verifying that the indexes are able to catch up with the chain-tip. Therefore, there is no need to run the process on a worker thread.
Note:
After #27607, the BaseIndex::Start method will be almost empty.
Only containing the ThreadSync call, allowing us to extract the
thread ownership from the index class and place it into an external
structure (probably, the thread pool introduced in #26966).
The tests objective is to exercise the chain synchronization
code, verifying that the indexes are able to catch up with the
chain-tip. Therefore, there is no need to run the process on
a worker thread.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | MarcoFalke, TheCharlatan |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
(An alternative, functionally equivalent to this, might be to just make the timeout infinite, see #27988 (comment), but I think this is fine as well.)
I think I’d prefer that option because then we wouldn’t need to add a test-only arg to BaseIndex::Start, plus having the same thread structure as in production seems more natural and more robust with respect to possible future changes of the init sequence and unit tests.
(An alternative, functionally equivalent to this, might be to just make the timeout infinite, see #27988 (comment), but I think this is fine as well.)
I think I’d prefer that option because then we wouldn’t need to add a test-only arg to
BaseIndex::Start, plus having the same thread structure as in production seems more natural and more robust with respect to possible future changes of the init sequence and unit tests.
I agree that the current index initialization structure isn’t ideal as it tightly couples the init sequence with the background sync thread creation.
We have #27607 addressing it, which decouples the init procedure from the BaseIndex::Start method. Leaving only the background sync thread creation there and nothing else.
Having that solved, and regarding the topic of matching production thread structure: I don’t see why unit tests in general should replicate the complete production behavior or precisely match the thread model used in production. Furthermore, none of these tests really concern themselves with concurrency; they simply start the index sync thread and wait for it to finish before proceeding, effectively running the task synchronously.
I mean, I’m also ok if we simply rise the timeout but, at the same time, I don’t think that the background threads creation adds any meaningful test coverage here.
On a slightly unrelated note and going a bit further over the topic, I think that the app’s threading decisions should not be part of the index class. In other words, threads should not be owned and managed by each index object. It is not a responsibility that indexes should have. Instead, indexes should sync purely based on external events.
And actually, by separating the threading concerns from the index class, we can optimize a high amount of redundant work currently performed by indexes. For instance, there is no need to traverse the entire chain and read every single block from disk in each of the index threads. We could have just one thread dedicated to reading blocks from disk, while an event dispatcher class notifies all existing indexes. Or.. even better, there could be multiple threads, reading different ranges of blocks at the same time, and dispatching events asynchronously to the indexes (this idea is related to #26966, and it’s something I intend to pursue after #24230).
I mean, I’m also ok if we simply rise the timeout but, at the same time, I don’t think that the background threads creation adds any meaningful test coverage here.
Right, it doesn’t add any meaningful coverage. Though, I think the main point is that we should try to avoid adding test-only code to “real” code where possible. I am fine with anything, though.
(Only modifying the test code would also avoid the conflicting with the other pull?)
I think I’d prefer that option because then we wouldn’t need to add a test-only arg to
BaseIndex::Start, plus having the same thread structure as in production seems more natural and more robust with respect to possible future changes of the init sequence and unit tests.
I think I (weakly) agree with this – having the code be non-threaded in tests but threading in the real use seems like an annoying difference.
Does your feedback from #28036 (comment) also apply here? I think you’ll also have to check for it being synced?
0Assert(index.GetSummary().synced);
🐙 This pull request conflicts with the target branch and needs rebase.
Does your feedback from #28036 (comment) also apply here? I think you’ll also have to check for it being synced?
0Assert(index.GetSummary().synced);
Yeah @MarcoFalke. Same applies here. I didn’t push it just because of #28036. Thought that we were going to tackle it there.
Already pushed #28044 fixing it.
Why not just push it here, and re-use this PR (dropping any now-redundant changes) . Otherwise, why is this still open.
Yeah, not sure why it got merged it.
It got merged to fix the persistent (potentially confusing) CI issues. What you want to fix isn’t currently an active issue.
Why not just push it here, and re-use this PR (dropping any now-redundant changes) . Otherwise, why is this still open.
Closed the PR a minute before your comment.
It got merged to fix the persistent (potentially confusing) CI issues. What you want to fix isn’t currently an active issue.
Well, the previous CI failure was due a pretty clear timeout error. Now there is a possibility of tests hanging forever with no clear reason. So, this last one is practically more confusing than the first one.
Isn’t a big deal anyway but would had being nicer if you would have waited a bit for Marco’s feedback there. The timeout was open for few days, it could had continued open for few more hours.
