Core: Replace Action class uses with GenericAction #31022

rjernst · 2018-06-01T05:24:41Z

Since #30966, Action no longer has anything but a call to the
GenericAction super constructor. This commit replaces all uses of Action
with GenericAction, and removes Action. It also removes Request as a
generic type of GenericAction, as it was unused.

Since elastic#30966, Action no longer has anything but a call to the GenericAction super constructor. This commit replaces all uses of Action with GenericAction, and removes Action. It also removes Request as a generic type of GenericAction, as it was unused.

elasticmachine · 2018-06-01T05:24:42Z

Pinging @elastic/es-core-infra

bleskes

LGTM.

nik9000

I wonder if we should rename GenericAction to Action now.

nik9000 · 2018-06-01T13:37:13Z

...es/reindex/src/test/java/org/elasticsearch/index/reindex/ReindexFromRemoteWithAuthTests.java


    /**
-     * Action filter that will reject the request if it isn't authenticated.
+     * GenericAction filter that will reject the request if it isn't authenticated.


Is this change intentional?

nik9000 · 2018-06-01T13:37:58Z

server/src/main/java/org/elasticsearch/action/ActionModule.java

            public <Request extends ActionRequest, Response extends ActionResponse> void register(
-                    GenericAction<Request, Response> action, Class<? extends TransportAction<Request, Response>> transportAction,
-                    Class<?>... supportTransportActions) {
+                GenericAction<Response> action, Class<? extends TransportAction<Request, Response>> transportAction,


Your indentation change here makes this harder to read.

nik9000 · 2018-06-01T13:38:21Z

server/src/main/java/org/elasticsearch/action/DocWriteRequest.java

 /**
 * Generic interface to group ActionRequest, which perform writes to a single document
- * Action requests implementing this can be part of {@link org.elasticsearch.action.bulk.BulkRequest}
+ * GenericAction requests implementing this can be part of {@link org.elasticsearch.action.bulk.BulkRequest}


I think this was right before?

nik9000 · 2018-06-01T13:39:17Z

server/src/main/java/org/elasticsearch/action/bulk/BulkRequest.java

                                fetchSourceContext = FetchSourceContext.fromXContent(parser);
                            } else {
-                                throw new IllegalArgumentException("Action/metadata line [" + line + "] contains an unknown parameter [" + currentFieldName + "]");
+                                throw new IllegalArgumentException("GenericAction/metadata line [" + line + "] contains an unknown parameter [" + currentFieldName + "]");


I think this was right before? Or, at least, this change doesn't make it more right.

nik9000 · 2018-06-01T13:39:35Z

server/src/main/java/org/elasticsearch/action/support/ThreadedActionListener.java

            this.logger = logger;
            this.threadPool = threadPool;
-             // Should the action listener be threaded or not by default. Action listeners are automatically threaded for
+             // Should the action listener be threaded or not by default. GenericAction listeners are automatically threaded for


Same here. I think they are still called action listeners.

nik9000 · 2018-06-01T13:47:41Z

...in/java/org/elasticsearch/xpack/core/watcher/transport/actions/stats/WatcherStatsAction.java


 /**
- * This Action gets the stats for the watcher plugin
+ * This GenericAction gets the stats for the watcher plugin


I think the old comment was fine.

nik9000 · 2018-06-01T13:47:50Z

...ugin/ml/src/main/java/org/elasticsearch/xpack/ml/job/persistence/JobDataCountsPersister.java

     * @param jobId Job to update
     * @param counts The counts
-     * @param listener Action response listener
+     * @param listener GenericAction response listener


I think the old comment was better.

nik9000 · 2018-06-01T13:48:02Z

...ugin/security/src/main/java/org/elasticsearch/xpack/security/authz/AuthorizationService.java

            // is this is performing multiple actions on the index, then check each of those actions.
            assert request instanceof BulkShardRequest
-                    : "Action " + action + " requires " + BulkShardRequest.class + " but was " + request.getClass();
+                    : "GenericAction " + action + " requires " + BulkShardRequest.class + " but was " + request.getClass();


I think the old message was fine,.

nik9000 · 2018-06-01T13:48:08Z

...ugin/security/src/main/java/org/elasticsearch/xpack/security/authz/AuthorizationService.java


            assert AuthenticateAction.NAME.equals(action) || HasPrivilegesAction.NAME.equals(action) || sameUsername == false
-                    : "Action '" + action + "' should not be possible when sameUsername=" + sameUsername;
+                    : "GenericAction '" + action + "' should not be possible when sameUsername=" + sameUsername;


I think the old message was fine.

nik9000 · 2018-06-01T13:48:26Z

...ty/src/test/java/org/elasticsearch/xpack/security/audit/index/IndexAuditTrailMutedTests.java

            protected <Request extends ActionRequest, Response extends ActionResponse, RequestBuilder extends
                    ActionRequestBuilder<Request, Response>> void doExecute(
-                    Action<Request, Response> action, Request request, ActionListener<Response> listener) {
+                GenericAction<Response> action, Request request, ActionListener<Response> listener) {


The indentation is not longer consistent here.

rjernst · 2018-06-18T18:23:27Z

@nik9000 I opened #31405 as an alternative to this PR, which does as you suggested (GenericAction -> Action). That also eliminates the IntelliJ wackiness that caused most of the doc changes you saw.

nik9000 · 2018-06-18T20:42:55Z

Thanks! I like that one a lot!

…

On Mon, Jun 18, 2018 at 2:23 PM Ryan Ernst ***@***.***> wrote: @nik9000 <https://github.com/nik9000> I opened #31405 <#31405> as an alternative to this PR, which does as you suggested (GenericAction -> Action). That also eliminates the IntelliJ wackiness that caused most of the doc changes you saw. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#31022 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AANLolHyj9NkecUHoklPPKskWGuVJh4yks5t9_AngaJpZM4UWHwc> .

rjernst added :Core/Infra/Core Core issues without another label v7.0.0 >refactoring labels Jun 1, 2018

bleskes approved these changes Jun 1, 2018

View reviewed changes

nik9000 reviewed Jun 1, 2018

View reviewed changes

rjernst added 2 commits June 18, 2018 07:23

iter?

a703c80

Merge branch 'master' into deguice32

0d8bfad

rjernst closed this Jun 18, 2018

rjernst deleted the deguice32 branch January 20, 2019 19:34

jimczi added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Core: Replace Action class uses with GenericAction #31022

Core: Replace Action class uses with GenericAction #31022

Uh oh!

Conversation

rjernst commented Jun 1, 2018

Uh oh!

elasticmachine commented Jun 1, 2018

Uh oh!

bleskes left a comment

Choose a reason for hiding this comment

Uh oh!

nik9000 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rjernst commented Jun 18, 2018

Uh oh!

nik9000 commented Jun 18, 2018 via email

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants