Skip to content
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

Refactor ElasticsearchClient, splitting into separate concerns for easier testing #9320

Closed
5 of 6 tasks
Tracked by #8609
npepinpe opened this issue May 6, 2022 · 0 comments · Fixed by #9359
Closed
5 of 6 tasks
Tracked by #8609

Refactor ElasticsearchClient, splitting into separate concerns for easier testing #9320

npepinpe opened this issue May 6, 2022 · 0 comments · Fixed by #9359
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

@npepinpe
Copy link
Member

npepinpe commented May 6, 2022

Description

The ElasticsearchClient is the one class in the exporter which communicates with the actual Elastic instance. Currently, it's responsibilities are:

  • Configures and manage the Elastic REST client
  • Network communication: (de)serailization, response/error handling, etc.
  • Read templates from the resources, parse them, and substitute certain configuration options
  • Compute the index name, document ID, etc., properties of each document.
  • Serialization of records and templates
  • Buffer serialized indexing commands in memory
  • Configurable logic for when flushing should occur

I would propose to do the following:

The client will now only deal with network communication and in memory buffering of the documents. I would recommend moving the flushing/buffering out into its own component as well later, but the above is what provides the biggest win, I think, in terms of maintainability and test-ability.

The last two steps will be blocked by the first three, I think, but the first three can be done in parallel. Finally, refactoring the tests may include adding new testing utilities, but these will later be reused in #9321.

@npepinpe npepinpe added kind/toil Categorizes an issue or PR as general maintenance, i.e. cleanup, refactoring, etc. team/process-automation area/test Marks an issue as improving or extending the test coverage of the project labels May 6, 2022
@npepinpe npepinpe self-assigned this May 6, 2022
zeebe-bors-camunda bot added a commit that referenced this issue May 10, 2022
9332: Extract template handling from ElasticsearchClient r=npepinpe a=npepinpe

## Description

This PR extracts the template handling logic (reading, parsing, configuring) logic from the `ElasticsearchClient` class into its own class. This will help increase maintainability and test-ability of how index templates are handled. The PR doesn't increase the test coverage marginally - for example, one could unit test the templates themselves (i.e. ensure fields are correctly typed). As this was not done previously, and it falls outside the scope of the KR, this is left as a recommendation.

## Related issues

closes #9328
related to #9320 



Co-authored-by: Nicolas Pepin-Perreault <nicolas.pepin-perreault@camunda.com>
zeebe-bors-camunda bot added a commit that referenced this issue May 10, 2022
9333: Extracts record index routing logic from ElasticsearchClient r=npepinpe a=npepinpe

## Description

This PR extracts the index routing logic from `ElasticsearchClient` into its own class, `RecordIndexRouter`. While it's very simple logic, since it defines the public API used by consumers of the exporter, it's useful to have it separate and well tested, even if the tests seem somewhat simple.

Part of the PR is refactoring some existing test utilities to make them work. These will go away at the end of the KR, so I wouldn't spend too much time on them. Similarly, once all the issues in #9320 are done, I will do a final pass to clean up the `ElasticsearchClient` class itself. Right now I tried to keep the refactoring to a minimal to create the PRs in parallel.

## Related issues

closes #9327 
related to #9320 



Co-authored-by: Nicolas Pepin-Perreault <nicolas.pepin-perreault@camunda.com>
@npepinpe npepinpe linked a pull request May 11, 2022 that will close this issue
10 tasks
@Zelldon Zelldon added the version:8.1.0 Marks an issue as being completely or in parts released in 8.1.0 label Oct 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants