-
Notifications
You must be signed in to change notification settings - Fork 421
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
Add high throughput integration test #5655
base: main
Are you sure you want to change the base?
Conversation
pub fn merge(self, other: RestIngestResponse) -> Self { | ||
Self { | ||
num_docs_for_processing: self.num_docs_for_processing + other.num_docs_for_processing, | ||
num_ingested_docs: apply_op(self.num_ingested_docs, other.num_ingested_docs, |a, b| { | ||
a + b | ||
}), | ||
num_rejected_docs: apply_op(self.num_rejected_docs, other.num_rejected_docs, |a, b| { | ||
a + b | ||
}), | ||
parse_failures: apply_op(self.parse_failures, other.parse_failures, |a, b| { | ||
a.into_iter().chain(b).collect() | ||
}), | ||
num_too_many_requests: self.num_too_many_requests, | ||
} | ||
} |
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 moved this back here as it makes more sense than in the API model because accumulating responses is quite specific to the rest client.
2f00367
to
de8f2a1
Compare
de8f2a1
to
aa98399
Compare
.ingest( | ||
index_id, | ||
IngestSource::Str(body), | ||
Some(5_000_000), |
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.
Casually observing.
Our batches tend to have 200 - 1000 items during bulk request and target 1 - 10 different indexes. The higher end is about 3mb. Here the typical rate of ingest is about 900mb/s
Our retry work load is capped to 500 items per batch and sits around 1mb. These would appear to touch 1 index. We are trying to determine if they are all the same index
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 playing with the batch size in this test because I had a surprising 500 (internal timeout) error. We want the system to behave consistently regardless of the batch size (obviously with very small batches adding a bit of overhead).
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.
What do we consider very small?
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.
One overhead of the top of my head is the http frame overhead. If you have more http headers than document payload, that's a bit of unnecessary extra work.
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.
the 1MB granularity should be pretty good (negligeable framing overhead in network exchanges but small enough to be nicely balanced between shards)
TODO BEFORE MERGING: figure out why the high throughput test fails with a big payload but not with smaller ones |
Description
This PR is a reuse of the tests and docs proposed in #5644, which itself is not necessary anymore after the the status code was fixed to be 429 when shards need scaling up (#5651)
How was this PR tested?
Describe how you tested this PR.