Make the high-level REST client skip types for document deletion. #34041
Conversation
I'm starting with delete since it is a bit simpler than other APIs, but we should eventually do the same with all other APIs that are being replaced with a typeless version (get, put_mapping, search, etc.). Relates elastic#15613
|
Pinging @elastic/es-core-infra |
|
Is the plan to migrate to new requests/responses as part of this change, or is that part necessary to not also affect transport client? I wonder because that's a breaking change for users, and I wonder what the plan is for all other requests/responses. Also, if we go ahead this way, we may want to remove response parsing code from |
|
Thanks for the feedback @javanna, this is the kind of comments that I was looking for. :)
It would be best if we could migrate the existing request/response classes, but unfortunately this is hard to do because types are only deprecated on the REST layer, not removed yet so the transport client needs to retain a way to set types on request objects, and there is already an
+1 We can't remove parsing logic from the old DeleteResponse class yet because of bulk requests, but I agree we should do this eventually. I'll add tests to the request/response classes. |
True, but if breaking was almost required for the transport client given the internal nature of the exposed classes, I hope at some point the REST client becomes stable. Obviously we are not there yet. Let's document this clearly and make sure that users know what to expect, it is true that the compiler helps but it's also good for users to know what they have to do to migrate before they go and do it. In a way it's good to show the direction, the delete API was added with 5.6 or earlier, and it will be updated to its own classes in 7.0 which is not bad at all. |
|
@javanna I pushed one more commit to add tests and document the breaking change. |
| import java.io.IOException; | ||
| import java.util.Collections; | ||
|
|
||
| public class DeleteResponseTests extends ESTestCase { |
There was a problem hiding this comment.
testing these new classes requires to repeat a lot of similar checks, that would be avoided with a test class like AbstractXContentTestCase. But this new class does not implement toXContent hence it's hard to do that. I have faced something similar in #33176 and added a possible approach. The bigger problem though is that we need to make sure that all the response parser support unknown fields (for forward compatibility) and test that, which is something that is very easy to forget if we extend ESTestCase. @hub-cap said he needs to take some time to think about this testing problem caused by using different objects in the client.
There was a problem hiding this comment.
Thanks for linking.
The bigger problem though is that we need to make sure that all the response parser support unknown fields
Indeed I actually explicitly disabled support for unknown fields, which is a mistake now that I read your comment.
| // Modify this statement once all document APIs have been migrated | ||
| `DeleteRequest` and `DeleteResponse` moved to the `org.elasticsearch.client` package | ||
| and do not reference types anymore. Under the hood, the high-level REST client |
There was a problem hiding this comment.
We also renamed all of their methods to make them proper getters I think.
|
Thanks for updating! I am good with this but there's a question mark on testing which requires @hub-cap 's thoughts, and his opinion on the overall direction would be great to have too. |
|
Thanks again for the feedback @javanna. No hurry on my end, I'm actually happy we're trying to think about how things should work since we'll have to do something similar for all other APIs that we are making type-less (put_mapping, get_mapping, index, get, put, bulk, update, etc.) and delete is probably one of the simpler ones. |
|
WRT the testing, we will have a conclusion very soon. Stay tuned. |
|
WRT breaking the existing API, im ok also with this until we get to 7.0, since its not technically released. Ill let others tell me this is a wrong approach tho cc @jasontedor for weighing in. |
| * @deprecated Use {@link RefreshPolicy} rather than {@link WriteRequest.RefreshPolicy}. | ||
| */ | ||
| @Deprecated | ||
| private static void setRandomLegacyRefreshPolicy(Consumer<WriteRequest.RefreshPolicy> setter, Map<String, String> expectedParams) { |
There was a problem hiding this comment.
Did this get deprecated and renamed? Should we be doing both?
@hub-cap just one small question, what do you mean with "its not technically released"? |
|
@javanna correct me if im wrong, but I assumed that we were "releasing" this in 7.0, and things prior to 7.0 were ok to change. If this is not the case then I definitely need to adjust my way of thinking!!! :) |
|
You should try and tell users that @hub-cap :) I guess for them , what they can use is released? |
|
haha, yea thats a fair point. I guess usable vs guaranteed back compat are 2 different things. Ill figure out what is the actual answer here. |
|
I think we can and have to break things, but let's be aware of when we do so and try to document such changes and help users to migrate? After all they have just made the effort of moving to our new client :) |
|
@jpountz would it be okay to close this, as we've decided to take a different approach to types deprecation and the Java HLRC? In particular, we are preferring to add deprecations to existing methods, as opposed to creating new requests + responses for every affected API. |
|
Absolutely. |
I'm starting with delete since it is a bit simpler than other APIs, but we
should eventually do the same with all other APIs that are being replaced with
a typeless version (get, put_mapping, search, etc.).
Relates #15613