Create version-specific helper for poison pills

[ldematte]

After discussion, we came to the agreement that keeping poison pills in code is beneficial; we also agreed to use a global "symbol" that is specific to a precise version, so we can stop using different flavors of comparisons based on Version.

The "symbol" we use is a public static function, for two reasons:

• it let us provide a reason message to help remember what the poison pill is about/what we need to fix
• it let us the freedom of modify the implementation later on, in case we need to make adjustments for specific cases (e.g. serverless - those should not be necessary, but just in case we need an additional tool)

The javadoc summarizes very briefly how we intend to use this assertion going forward, making it trip on a dedicated branch in order to find and address all the poison pill before starting the release process.

Update: second take, using an annotation.

As of David suggestion, I skipped any "reason" or "issueUrl" for now.
I also went on and replaced some assert Version... with the new annotation.
The annotation can be less flexible and force you to refactor the code to change/replace into a separate method (see the examples). I do not think this is a bad thing, but I'll let you judge.

For the reviewers:

• Please comment on the javadoc comment: does it says enough?
• Should we use a free text reason or an issueUrl (like in the AwaitsFix annotation)? The asserta I replaced all had a message or a short comment. I kept it as a short comment for now.

Related to ES-6873

[DaveCTurner]

I'd be inclined to put this in its own class. It's not really an assertion, there's no need to even call it, it's just a symbol to reference in all the places that need cleaning up. In particular it's useful sometimes just to mention it in Javadocs.

I also don't think it needs a String reason argument. In most cases it'll be obvious from the context why it's there, and if not then that's something that deserves some more extensive commentary than we'd normally put into a string literal.

[ldematte]

I'd be inclined to put this in its own class. It's not really an assertion

No problem here. But based on the team comments, I am inclined to keep the "assert" in the name at least, so the behaviour is clear (will only have effects when running with assertions on, will throw an AssertionException, ...)

there's no need to even call it, it's just a symbol to reference in all the places that need cleaning up

That's why I was leaning towards an annotation, to make it more visible and visible "earlier" (even in precommit, for example). Calling it (vs just mentioning it in Javadoc) will enforce that we address it before releasing, it can have some value.

I also don't think it needs a String reason argument. In most cases it'll be obvious from the context why it's there, and if not then that's something that deserves some more extensive commentary than we'd normally put into a string literal.

What about using a issueUrl instead? It'll give you space for more extensive commentary, link to other issues, etc.

[ldematte]

BTW, any suggestion on were to place it? Since we are moving it from Assertions, I wonder if libs.core is the right place, or if server (e.g. in the org.elasticsearch namespace) is better.

[DaveCTurner]

libs:org.elasticsearch.core seems fine to me.

[DaveCTurner]

(will only have effects when running with assertions on, will throw an AssertionException, ...)

But it doesn't have any effects, it's an assert true. We're not going to make it an assert false, we're just going to remove it when the time comes.

That's why I was leaning towards an annotation

I'd be happy with an annotation. You can mention annotations in Javadocs easily enough. Possibly as well as just a plain symbol.

What about using a issueUrl instead?

I'm still leaning towards YAGNI here. I think folks are good enough at writing comments as needed in these cases, possibly linking to issues from the comment if a link is needed, but often it won't be and that's fine. Let's try without and if we see that it causes problems in practice then we can rethink.

[DaveCTurner]

I also suggest you adjust some of the existing poison pills to use this new functionality in this very PR so we can get a better sense of how it works in practice.

[thecoop]

I think an annotation would work a lot better here - its a declarative statement about code, not part of any functionality. You can even declare the annotation as source-file only, so it doesnt even appear in compiled class files.

[ldematte]

We're not going to make it an assert false,

I thought we were, so we are sure we got all of them. But yeah, if we remove it entirely we won't compile, so no point.

[ldematte]

I'll give annotations a go then :)

[ldematte]

Based on our conversations, I did not add an annotation processor, just the annotation

[DaveCTurner]

I like it :)

[DaveCTurner]

We should make this setting DeprecatedCritical if it's going away.

[thecoop]

To make this a bit more general, how about @updateForV9? Then it doesnt indicate the component needs to be removed completely, just updated.

[ldematte]

I was also thinking that "Remove" is not always precise. @UpdateForV9 is OK even if it's a little less "strong", but it may be more applicable in more cases (e.g. many are "replace a warning/a default with an exception", so "removal" is a bit off there)

[elasticsearchmachine]

Pinging @elastic/es-core-infra (Team:Core/Infra)

[elasticsearchmachine]

Hi @ldematte, I've created a changelog YAML for you.

[rjernst]

While the annotation seems mostly ok, I think we are losing some things by this not being an actual check.

• Currently with the existing assertions, when we bump the next major, we must address all of these poison pills. While a lot of these are cleanup, I thought I've seen them before where they address behavior (eg code assumes something that no longer holds true). I can't find any right now, but if that is a valid use case for these pills then not having a check means when we bump we could merge to main, and thus push code violating those assumptions to serverless. With an assertion we ensure these must be addressed in a feature branch before merging to main.
• Can the annotation be placed on any line of code? I don't think it can, I think it must be attached to something. So we lose the flexibility to keep note that something needs to be changed next to the thing that actually needs changing. As an example, look at the poison pills in ReplicationTracker or TransportStats.

[ldematte]

Addressing this one:

Can the annotation be placed on any line of code? I don't think it can, I think it must be attached to something. So we lose the flexibility to keep note that something needs to be changed next to the thing that actually needs changing. As an example, look at the poison pills in ReplicationTracker or TransportStats.

You are right, it's the first thing I noticed when I switched to annotations and tried to address a few like David suggested. You have to refactor the code a bit to isolate the lines you want to flag for removal/change in a method or class. I don't think this is bad though - it can make clearer which bit of code you need to work on, and make life easier when time for change/removal comes.

That said, I'm not strongly towards static functions or annotations, neither I am strongly pro using them just as a placeholder symbol or actually have additional logic (assert false).
If we want annotations AND logic we can do that too - annotations can have processors and run with or without them, and we can have logic in the processor.

[DaveCTurner]

I've seen them before where they address behavior (eg code assumes something that no longer holds true)

I would expect it almost always to be the other way around: the code is there to handle something that we can (in the future version) assume not to need handling any more. I'd be very interested in examples where that isn't true. But in any case I expect it's enough to have a reminder (such as this annotation) which we would remove when we've taken whatever action needs taking.

So we lose the flexibility to keep note that something needs to be changed next to the thing that actually needs changing.

I am not wholly convinced we need this flexibility. I see some benefits in forcing the relevant code to be isolated in its own method, and I'm sure we can always find something to annotate even where it's tricky to isolate exactly the relevant code.

look at the poison pills in ReplicationTracker or TransportStats.

I only see one in ReplicationTracker, in loadRetentionLeases, and I think we can reasonably do that with an annotation. The ones in TransportStats should really be phrased in terms of TransportVersion.

[ldematte]

@elasticmachine rerun elasticsearch-ci/part-3

[rjernst]

LGTM

[ldematte]

Thanks all for all the input!