-
Notifications
You must be signed in to change notification settings - Fork 24.9k
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
Name BulkItemResponse
ctors
#76439
Name BulkItemResponse
ctors
#76439
Conversation
Pinging @elastic/es-distributed (Team:Distributed) |
`BulkItemResponse` can contain either a success or failure. This replaces the two constructors used to build either case with named static methods. So instead of ``` return new BulkItemResponse(0, OpType.CREATE, createResponse); return new BulkItemResponse(0, OpType.CREATE, failure); ``` you now use ``` return BulkItemResponse.success(0, OpType.CREATE, createResponse); return BulkItemResponse.failure(0, OpType.CREATE, failure); ``` This makes it marginally easier to read code building these things - you don't have to know the type of the parameter to know if its a failure or success.
private Failure failure; | ||
|
||
BulkItemResponse() {} | ||
private final Failure failure; |
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.
This all started with me wanting to make these final to make them easier to reason about.
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.
LGTM! Left one small comment
this.id = id; | ||
this.response = response; | ||
this.opType = opType; | ||
this.failure = failure; |
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 think it makes sense to add an assertion here that either response
or failure
needs to be set, but not both.
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.
👍
Thanks for reviewing @arteam. |
💔 Backport failed
To backport manually run |
Manual backport incoming. |
`BulkItemResponse` can contain either a success or failure. This replaces the two constructors used to build either case with named static methods. So instead of ``` return new BulkItemResponse(0, OpType.CREATE, createResponse); return new BulkItemResponse(0, OpType.CREATE, failure); ``` you now use ``` return BulkItemResponse.success(0, OpType.CREATE, createResponse); return BulkItemResponse.failure(0, OpType.CREATE, failure); ``` This makes it marginally easier to read code building these things - you don't have to know the type of the parameter to know if its a failure or success.
High touch backport! Just about every line conflicted because master doesn't have types. Fun. |
`BulkItemResponse` can contain either a success or failure. This replaces the two constructors used to build either case with named static methods. So instead of ``` return new BulkItemResponse(0, OpType.CREATE, createResponse); return new BulkItemResponse(0, OpType.CREATE, failure); ``` you now use ``` return BulkItemResponse.success(0, OpType.CREATE, createResponse); return BulkItemResponse.failure(0, OpType.CREATE, failure); ``` This makes it marginally easier to read code building these things - you don't have to know the type of the parameter to know if its a failure or success.
BulkItemResponse
can contain either a success or failure. Thisreplaces the two constructors used to build either case with named
static methods. So instead of
you now use
This makes it marginally easier to read code building these things - you
don't have to know the type of the parameter to know if its a failure
or success.