-
Notifications
You must be signed in to change notification settings - Fork 25.5k
[Backport] Streams - Log's Enable, Disable and Status endpoints (#129474) #129838
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Backport] Streams - Log's Enable, Disable and Status endpoints (#129474) #129838
Conversation
* Enable And Disable Endpoint * Status Endpoint * Integration Tests * REST Spec * REST Spec tests * Some documentation * Update docs/changelog/129474.yaml * Fix failing security test * PR Fixes * PR Fixes - Add missing feature flag name to YAML spec * PR Fixes - Fix support for timeout and master_timeout parameters * PR Fixes - Make the REST handler validation happy with the new params * Delete docs/changelog/129474.yaml * PR Fixes - Switch to local metadata action type and improve request handling * PR Fixes - Make enable / disable endpoint cancellable * PR Fixes - Switch timeout param name for status endpoint * PR Fixes - Switch timeout param name for status endpoint in spec * PR Fixes - Enforce local only use for status action * PR Fixes - Refactor StreamsMetadata into server * PR Fixes - Add streams module to multi project YAML test suite * PR Fixes - Add streams cluster module to multi project YAML test suite
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR backports new functionality to support enabling, disabling, and querying the status of logs streams. Key changes include the introduction of the StreamsMetadata for storing logs state, new REST and transport actions for toggling and retrieving log streams status, and corresponding tests and API spec updates.
Reviewed Changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
x-pack/plugin/security/qa/operator-privileges-tests/src/javaRestTest/java/org/elasticsearch/xpack/security/operator/Constants.java | Adds necessary privilege constants for the new streams endpoints |
server/src/main/java/org/elasticsearch/cluster/metadata/StreamsMetadata.java | Introduces the streams metadata to track logs state |
server/src/main/java/org/elasticsearch/cluster/ClusterModule.java and TransportVersions.java | Registers the streams metadata and adds a new transport version constant |
rest-api-spec/* | Defines new API endpoints for logs stream enable, disable, and status operations |
modules/streams/src/main/java/org/elasticsearch/rest/streams/logs/* | Implements the REST and transport actions for streams status and logs toggling |
modules/streams/* | Adds tests, plugin support, and build configurations for the streams feature |
modules/streams/src/main/java/org/elasticsearch/rest/streams/logs/StreamsStatusAction.java
Show resolved
Hide resolved
|
||
@Override | ||
public TransportVersion getMinimalSupportedVersion() { | ||
return TransportVersions.STREAMS_LOGS_SUPPORT_8_19; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume this is OK as although this isn't technically true (9.0.x nodes aren't supported), we won't allow 9.0 nodes to join a 8.19 cluster?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean, technically it is true that the _8_19 version is the minimal supported one — what's not true is that all higher versions support it. In other words, it's the default supportsVersion()
which is technically not telling the truth. But, as you say, this should probably be okay?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My understanding of getMinimalSupportedVersion
is that we will send this to 9.0 node with this change - but that's fine because 8.19 isn't compatible with 9.0 at all - needs 9.1. We have all kinds of wire incompatibilities between 8.19 and 9.0 already.
So is this true? From a certain point of view.
Maybe you'd best leave a comment saying this so folks reading won't be confused. It's possible that in a year we'll have forgotten this weird version problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a clean backport apart from the transport version stuff? If so, this LGTM, but once again let's have Core/Infra confirm we're doing the write thing there.
|
||
@Override | ||
public TransportVersion getMinimalSupportedVersion() { | ||
return TransportVersions.STREAMS_LOGS_SUPPORT_8_19; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean, technically it is true that the _8_19 version is the minimal supported one — what's not true is that all higher versions support it. In other words, it's the default supportsVersion()
which is technically not telling the truth. But, as you say, this should probably be okay?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Transport version dance looks right to me. I can't approve the rest, but y'all have that covered.
|
||
@Override | ||
public TransportVersion getMinimalSupportedVersion() { | ||
return TransportVersions.STREAMS_LOGS_SUPPORT_8_19; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My understanding of getMinimalSupportedVersion
is that we will send this to 9.0 node with this change - but that's fine because 8.19 isn't compatible with 9.0 at all - needs 9.1. We have all kinds of wire incompatibilities between 8.19 and 9.0 already.
So is this true? From a certain point of view.
Maybe you'd best leave a comment saying this so folks reading won't be confused. It's possible that in a year we'll have forgotten this weird version problem.
Backport of #129474. Currently blocked by #129796.
Thanks to @masseyke for 99.9% of the leg work on this with #129798