Using the STREAMS_LOGS_SUPPORT_8_19 transport version#129796
Using the STREAMS_LOGS_SUPPORT_8_19 transport version#129796lukewhiting merged 5 commits intoelastic:mainfrom
Conversation
|
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.
PeteGillinElastic
left a comment
There was a problem hiding this comment.
In addition to my question above, there are some test failures which appear related to this PR: TestToggleIT.testLogStreamToggle, and a YAML REST test Basic toggle of logs state enable to disable and back.
nik9000
left a comment
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 <luke.whiting@elastic.co>
This follows #129753, using the transport version it introduced.