-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Streams - Log's Enable, Disable and Status endpoints #129474
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
Streams - Log's Enable, Disable and Status endpoints #129474
Conversation
|
Hi @lukewhiting, I've created a changelog YAML for you. |
|
Pinging @elastic/es-data-management (Team:Data Management) |
rest-api-spec/src/main/resources/rest-api-spec/api/streams.logs_disable.json
Outdated
Show resolved
Hide resolved
...treams/src/main/java/org/elasticsearch/rest/streams/logs/RestSetLogStreamsEnabledAction.java
Outdated
Show resolved
Hide resolved
rest-api-spec/src/main/resources/rest-api-spec/api/streams.status.json
Outdated
Show resolved
Hide resolved
nielsbauman
left a comment
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 didn't look much at the logic or tests, I just left some comments on the general wiring of the new APIs.
.../streams/src/main/java/org/elasticsearch/rest/streams/logs/TransportStreamsStatusAction.java
Outdated
Show resolved
Hide resolved
modules/streams/src/internalClusterTest/java/org/elasticsearch/rest/streams/TestToggleIT.java
Show resolved
Hide resolved
...treams/src/main/java/org/elasticsearch/rest/streams/logs/RestSetLogStreamsEnabledAction.java
Outdated
Show resolved
Hide resolved
nielsbauman
left a comment
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.
Aside from the timeout comment, the API wiring stuff LGTM 👍
|
|
||
| @Override | ||
| protected BaseRestHandler.RestChannelConsumer prepareRequest(RestRequest request, NodeClient client) { | ||
| StreamsStatusAction.Request statusRequest = new StreamsStatusAction.Request(RestUtils.getAckTimeout(request)); |
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.
This should use the masterNodeTimeout (same goes for the supported params above). Even though the request runs on the local node, we decided to keep the query param name the same (master_timeout) for BwC reasons, so I think it makes sense to use that name here too for consistency.
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.
Switched both to master node timeout 👍🏻
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.
Don't forget to also update the spec :)
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.
Too many moving parts 😅 Thanks for the reminder
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 introduces a new streams module that provides REST endpoints to enable, disable, and report the status of the logs streams feature, backed by a cluster-state metadata custom object and tested via both YAML and Java integration tests.
- Add REST API specs, handlers, and transport actions for
/_streams/logs/_enable,/_streams/logs/_disable, and/_streams/status. - Persist feature state in a new
StreamsMetadatacluster-state custom object using a sequential, acking task executor. - Register the module in the build and multi-project test configurations, and add both YAML and internal-cluster tests.
Reviewed Changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| x-pack/qa/multi-project/core-rest-tests-with-multiple-projects/build.gradle | Include :modules:streams in YAML REST multi-project tests |
| x-pack/plugin/security/qa/operator-privileges-tests/.../Constants.java | Grant operator privileges for streams toggle and status |
| server/src/main/java/org/elasticsearch/cluster/SequentialTaskAckingTaskExecutor.java | New executor for sequential acked cluster-state update tasks |
| server/src/main/java/org/elasticsearch/TransportVersions.java | Add STREAMS_LOGS_SUPPORT transport version constant |
| rest-api-spec/src/main/resources/rest-api-spec/api/streams.*.json | Define API specs for streams enable, disable, and status |
| modules/streams/src/yamlRestTest/.../streams/logs/10_basic.yml | Add YAML tests for toggling and status checks |
| modules/streams/src/main/java/org/elasticsearch/rest/streams/logs/Transport*Action.java | Implement transport actions and REST handlers |
| modules/streams/src/main/java/org/elasticsearch/rest/streams/StreamsPlugin.java | Register REST handlers, actions, and named writeables |
| modules/streams/src/main/java/org/elasticsearch/rest/streams/StreamsMetadata.java | Define custom cluster-state metadata for streams feature |
| modules/streams/src/main/java/module-info.java | Module descriptor for the streams plugin |
| modules/streams/src/internalClusterTest/.../TestToggleIT.java | Add Java integration test for toggle behavior |
Comments suppressed due to low confidence (3)
modules/streams/src/main/java/module-info.java:10
- The module name
org.elasticsearch.rest.rootis misleading; consider renaming it toorg.elasticsearch.rest.streamsto match the plugin package and functionality.
module org.elasticsearch.rest.root {
modules/streams/src/main/java/org/elasticsearch/rest/streams/logs/TransportLogsStreamsToggleActivation.java:85
- [nitpick] The task name prefix
enable-streams-logscan be confusing when disabling; consider using a neutral prefix likestreams-logs-toggle-[%s]so both enable and disable modes read clearly.
String taskName = String.format(Locale.ROOT, "enable-streams-logs-[%s]", shouldEnable ? "enable" : "disable");
modules/streams/src/main/java/module-info.java:12
- The
@Injectannotation fromorg.elasticsearch.injection.guiceis used throughout the code but the module descriptor does not require the injection module; addrequires org.elasticsearch.injection.guice(or the correct module) to avoid compilation errors.
requires org.elasticsearch.xcontent;
|
|
||
| @Override | ||
| public void writeTo(StreamOutput out) throws IOException { | ||
| out.writeBoolean(logs_enabled); |
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.
This ought to never be called, right? You could just add an assert false or something.
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.
Good catch! You can use TransportAction.localOnly() for that, if you want.
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.
Replaced this with a call to TransportAction.localOnly() and removed the redundant constructor.
| logs_enabled = logsEnabled; | ||
| } | ||
|
|
||
| public Response(StreamInput in) throws IOException { |
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.
This is never called, right? And this is always local? So I think you could just delete it.
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.
Removed :-)
| */ | ||
| public class SequentialTaskAckingTaskExecutor<Task extends AckedClusterStateUpdateTask> extends SimpleBatchedAckListenerTaskExecutor<Task> { | ||
| @Override | ||
| public Tuple<ClusterState, ClusterStateAckListener> executeTask(Task task, ClusterState clusterState) throws Exception { |
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.
Just talking myself through what you've probably already considered -- there's no need to use a AckedBatchedClusterStateUpdateTask and to batch (deduplicate) these, b/c this is just not an action that is commonly going to ever be called more than once in the lifetime of a 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.
@masseyke This executor class extends SimpleBatchedAckListenerTaskExecutor, so these cluster state updates will be batched. Maybe the name (SequentialTaskAckingTaskExecutor) should include the word "batch" somewhere to indicate that?
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.
Hmm I think I'm just confused between what AckedBatchedClusterStateUpdateTask and AckedClusterStateUpdateTask are for. But regardless, it looks fine.
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.
Renamed this class to SequentialAckingBatchedTaskExecutor to avoid future confusion
| ActionListener<AcknowledgedResponse> listener | ||
| ) throws Exception { | ||
| ProjectId projectId = projectResolver.getProjectId(); | ||
| StreamsMetadata streamsState = state.projectState(projectId).metadata().custom(StreamsMetadata.TYPE, StreamsMetadata.EMPTY); |
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.
Since StreamsMetadata is in a module, and we need to be able to check this from server (within the bulk transport action), do we need to move StreamsMetadata to server? Or have some way to expose it in there?
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.
Yep that's probably a good idea. Have moved the metadata object into server.
| restTestConfig project(path: ':modules:data-streams', configuration: "basicRestSpecs") | ||
| restTestConfig project(path: ':modules:ingest-common', configuration: "basicRestSpecs") | ||
| restTestConfig project(path: ':modules:reindex', configuration: "basicRestSpecs") | ||
| restTestConfig project(path: ':modules:streams', configuration: "basicRestSpecs") |
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 think you've also got to have
clusterModules project(':modules:streams')
masseyke
left a comment
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.
LGTM once the build works.
* 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
* 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
) (#129838) * Streams - Log's Enable, Disable and Status endpoints (#129474) * 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 * Null out reader in status transport action super constructor * Added comment about minimum version support
* 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
* 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
This pull request introduces a new module to Elasticsearch called
streams.This feature will provide "batteries included" ingest of bulk data with minimal user setup.
This initial PR introduces the new new ES module to support the function, including:
Impliments ES-11330