Elasticsearch: Java api: move to ordinary getters and setters

[javanna]

We've been talking for some time about whether we should start using ordinary getters and setters like any other java project rather than our current methods that don't use the get and set prefix. We have also been moving to ordinary getters and setters as we go, but for the java api it would be beneficial to move over all of the objects at this point, otherwise we just introduce or keep inconsistencies.

[s1monw]

hell yeah!!!

[MaineC]

@javanna what's our preferred strategy here:

• Simply switch over in v5.0.0 (cheapest, IDE will warn anyway),
• Deprecate in 2.x, and remove in v5.x (means adding a dependency to deprecate everything that should be removed before 2.x is no longer supported),
• Deprecate in 5.x and remove one version later (means carrying the old syntax along for one more release)?

I have forgotten how we've handled similar cases in the past, I'm afraid - I believe it was option one, but am not entirely certain.

[javanna]

I think that we can go and remove in 5.0 directly given that our java users have a compiler which will tell them what they have to change. This is the same rationale followed with the search refactoring. Let's document the changes though. Seems overkill to introduce deprecations that will stick around for the whole 5.x series, and it's too late to introduce deprecations in 2.x.

[MaineC]

+1 Thanks for the clarification.

[nik9000]

Maybe we don't should just do it in on the REST java api and not bother with the native one?

[javanna]

I think that we should clean things up regardless of whether the java api will be replaced with something else or not. The request builders are the ones that may go at some point, but they only have proper setters so nothing to do there. I think this is about request objects, which will stay around for internal use anyways, and should be cleaned up at this point. My 2 cents though, we can discuss :)

[MaineC]

tl;dr: Both good points, me personally I'm in favour of getting rid of inconsistencies. It makes it easier to add new code that is in line with what we want to see by looking around yourself.

Longer version in addition to the above: While having consistent code is a nice goal, tidying stuff up that will be deleted in a few days or weeks isn't particularly satisfying. Remaining open questions might be:

• Which classes should be included in the cleanup - stuff that gets kicked out soon probably isn't worth looking at.
• Should we use a "switch as we go" strategy like we did before?

[javanna]

I think this is about request objects

By that I meant classes that extend ActionRequest. Those will stay anyways and are the ones with most inconsistencies I think.

[javanna]

Also classes that extend ActionResponse should be targeted I think. Do we want to discuss this before we start, just so we agree we should go ahead? We can mark discuss and discuss it in the next fixit friday if needed.

[MaineC]

Do we want to discuss this before we start, just so we agree we should go ahead?

Yeah - that actually was why instead of pinging people privately I put my initial question about how to proceed to this issue.

[elasticmachine]

Pinging @elastic/es-core-infra

[martijnvg]

This is still relevant for high level rest client and needs a discussion.

[nik9000]

I'd like to do this, but later. I feel like asking folks to move to the high level rest client is enough breaking changes for the 6.x series. I'd like us to have another look at this for 7.0 or even 8.0 depending on timing.

While I'm thinking about this: I'm fairly sure we shouldn't do this as a "big bang" thing. It'll amount to a huge change that'll be impossible to review. We should (and do) try to stick with setters on new code.

[rjernst]

We have had many discussions over the last few months on this. The biggest problem is switching the entire codebase at once is not possible. The best we can do is agree on a set of rules that can be pointed to in code reviews, and slowly migrating existing code.

The following is a concrete proposal for that ruleset:

• Members that are mutable should have getX() and setX(x) methods
• Members that are immutable should have a x() getter (notice no get prefix). This is to easily differentiate from the mutable members by the accessor, without having to look at the signature of the member.

[kaliljoao]

Hy guys, can I work on this issue?