Using the STREAMS_LOGS_SUPPORT_8_19 transport version #129796
Conversation
masseyke
added
>non-issue
:Data Management/Data streams
elasticsearchmachine
added
the
Team:Data Management
|
Pinging @elastic/es-data-management (Team:Data Management) |
Returning null from getMinimalSupportedVersion
| @Override | ||
| public TransportVersion getMinimalSupportedVersion() { | ||
| return TransportVersions.STREAMS_LOGS_SUPPORT; | ||
| return null; |
There was a problem hiding this comment.
Are you sure that it's safe to return null here? It has a few callers outside of the default implementation of supportsVersion(), including in AbstractNamedDiffable.diff() (directly or indirectly, on both code paths).
I see a couple of different patterns elsewhere in the codebase where supportVersion() is overridden.
MinimalServiceSettings.supportsVersionandModelRegistryMetadatahave the same logic to what you have here, andgetMinimalSupportedVersion()returns the_8_19version.AggregateMetricDoubleBlockBuilder,NoneChunkingSetting, andPinnedRankDocmake this method, e.g. the first of those doesthrow new UnsupportedOperationException("must not be called when overriding supportsVersion").
(There's alsoTimeSeriesSourceOperator.Statuswhere the two methods don't seem to agree on the minimal version, I haven't looked properly at that.)
If we're sure this method will never be called, I would prefer to throw an exception with an explicit message like e.g. AggregateMetricDoubleBlockBuilder does, so that if we're wrong we'll get something more helpful than an NPE somewhere later on. I don't know whether that or returning the _8_19 version is safer.
Hopefully someone from Core/Infra can sort us out here?
There was a problem hiding this comment.
We tried throwing an exception originally but it causes the tests to fail too. It seems returning TransportVersions.STREAMS_LOGS_SUPPORT_8_19 fixes the test fails so that looks to be like it's the right path possibly?
There was a problem hiding this comment.
Since there is a precedent for doing it, it fixes the tests, let's maybe go with this and see what Core/Infra have to say.
There was a problem hiding this comment.
FWIW, I'm throwing an exception in this somewhat related PR, and that works fine.
There was a problem hiding this comment.
The change looks fine.
If this weren't an 8.19 transport version we'd need to be mega sure we didn't cut a serverless release with the version. But serverless won't need the version anyway - it'll always be onOrAfter(STREAMS_LOGS_SUPPORT) because it's based off of main.
* Using the STREAMS_LOGS_SUPPORT_8_19 transport version * Update StreamsMetadata.java Returning null from getMinimalSupportedVersion * Return minimal supported version as 8.19 for metadata object to fix test fail --------- Co-authored-by: Luke Whiting <[email protected]>
6 participants
This follows #129753, using the transport version it introduced.