Refactor client's integration tests #9331
Assignees
Labels
area/test
Marks an issue as improving or extending the test coverage of the project
kind/toil
Categorizes an issue or PR as general maintenance, i.e. cleanup, refactoring, etc.
version:8.1.0-alpha2
version:8.1.0
Marks an issue as being completely or in parts released in 8.1.0
Comments
6 tasks
npepinpe
added
kind/toil
This was referenced May 10, 2022
10 tasks
ghost
pushed a commit
that referenced
this issue
May 12, 2022
9345: Extract record buffering logic out of ElasticsearchClient r=npepinpe a=npepinpe ## Description Initially I was only going to clean up and simplify that part, but I think I'd rather leave the client to only deal with building/sending requests, and handling responses/failures to/from Elastic. If you follow commit by commit, I think you can see how I ended up rolling this together. This PR extracts the record buffering logic out of `ElasticsearchClient` and into its own component, `BulkIndexRequest`. This class will take care of indexing records, serializing them, and serializing itself at the end as a line separated list of bulk actions (see https://www.elastic.co/guide/en/elasticsearch/reference/7.17/docs-bulk.html for more). Using a specific DTO for the bulk action (`BulkIndexAction`) makes it clearer what it will be serialized as, and allows us to write simpler assertions for our tests. Similarly, splitting the `BulkIndexRequest` away from the client makes it easier to understand how the request is built up and serialized, while also making the client (and thus exporter) much more observable. I opted against serializing the metadata of the bulk action, primarily because it has somewhat of an upper bound on how big it can get, and I expect the bulk of the memory usage to be records related. Leaving deserialized makes it much more observable. The one downside I see there is that if there was a serialization error during flush, it's a bit late for that, but I think that's a very unlikely case. There's some overlap at the moment between the tests, which I will deal with in #9330 and #9331. ## Related issues closes #9340 Co-authored-by: Nicolas Pepin-Perreault <nicolas.pepin-perreault@camunda.com>
ghost
closed this as 
ChrisKujawa
added
the
version:8.1.0
This issue was closed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
No description provided.
The text was updated successfully, but these errors were encountered: