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

Change return type of executing bulk request #205

Merged
merged 7 commits into from
May 9, 2023

Conversation

jlcanela
Copy link
Contributor

@jlcanela jlcanela commented May 6, 2023

A good practice with ElasticSearch when bulk insert is to verify the status for each Bulk response item so that it’s possible to retry this item associated request if necessary. Current bulk request returning Unit prevents such practice.

This PR:

  • updates executeBulk to return a BulkResponse instead of Unit
    • new BulkResponse case class in zio.elasticsearch.executor.response package
    • Note: all related case classes are in the same file, maybe you’d rather have each case class in its own file ?
  • updates tests
    • HttpElasticExecutorSpec, now ensure a BulkResponse is returned instead of Unit
    • it/HttpExecutorSpec, now ensure BulkResponse returns bulk request status including error case

Copy link
Member

@drmarjanovic drmarjanovic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @jlcanela,

I would like to thank you for your contribution! Amazing work overall!

Besides several remarks in the code, I would like to point out that BulkResponse is a package private class, and therefore, I'm not sure it can be used from the app - you can try it in our example app.

We have private[elasticsearch] for internal Elastic responses, and our practice is to have a separate class for the library responses - you can check the result package. We should identify what is the most important info for the bulk response and define BulkResult which will be public.

@jlcanela
Copy link
Contributor Author

jlcanela commented May 8, 2023

Hey @jlcanela,

I would like to thank you for your contribution! Amazing work overall!

Besides several remarks in the code, I would like to point out that BulkResponse is a package private class, and therefore, I'm not sure it can be used from the app - you can try it in our example app.

We have private[elasticsearch] for internal Elastic responses, and our practice is to have a separate class for the library responses - you can check the result package. We should identify what is the most important info for the bulk response and define BulkResult which will be public.

Thank you for your kind remarks.
I now use similar patterns to 'result' package. Don’t hesitate if you have any comment.

@dbulaja98 dbulaja98 changed the title Bulk request now returns BulkResponse instead of Unit Change return type of executing bulk request May 9, 2023
@dbulaja98 dbulaja98 merged commit e94deea into lambdaworks:main May 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants