Skip to content
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

Add Failure Details to every NodesResponse #17964

Conversation

pickypg
Copy link
Member

@pickypg pickypg commented Apr 25, 2016

Most of the current implementations of BaseNodesResponse (plural Nodes) ignore FailedNodeExceptions.

  • This adds a helper function to do the grouping to TransportNodesAction
  • Requires a non-null list of generic NodeResponses
  • Requires a non-null list of FailedNodeExceptions within the BaseNodesResponse constructor
  • Reads/writes the lists to Streamable
  • Also adds StreamInput and StreamOutput methods for generically reading and writing Lists of Streamables

/cc @jasontedor (for failures) @nik9000 (for StreamInput/Output helpers)

Closes #3740

@pickypg pickypg added review :Core/Infra/Core Core issues without another label v5.0.0-alpha2 labels Apr 25, 2016
*/
@SuppressWarnings("unchecked")
public ResponseContainer(Class<T> clazz, List<T> nodeResponses, List<FailedNodeException> failures) {
this.nodeResponses = nodeResponses.toArray((T[])Array.newInstance(clazz, nodeResponses.size()));
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really want to pass through IntFunction<T[]> arrayCreator for every single one, but I don't really like this variant either. Curious how others feel.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd consider converting everything in BaseNodesResponse to a list instead of an array and then just passing the list around?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd consider converting everything in BaseNodesResponse to a list instead of an array and then just passing the list around?

@pickypg Can we do this?

@jasontedor
Copy link
Member

@pickypg Other than the ToXContent stuff that we discussed via another channel, I left a comment about the ResponseContainer abstraction; I do not think this is necessary and left a specific suggestion. I'll hold off on a thorough review until you have a chance to process those two things.

@jasontedor
Copy link
Member

Note that this will close #3740.

@clintongormley clintongormley added v5.0.0-alpha3 :Data Management/Stats Statistics tracking and retrieval APIs >enhancement and removed v5.0.0-alpha2 :Core/Infra/Core Core issues without another label labels Apr 26, 2016
@pickypg pickypg force-pushed the feature/always-record-base-nodes-response-failures branch from 906f804 to c0001a4 Compare April 27, 2016 04:26
@pickypg
Copy link
Member Author

pickypg commented Apr 27, 2016

@jasontedor Updated. I force pushed as it touched a lot of the existing code. I do prefer the abstract method. I didn't think that every instance was using it, but they were, so I definitely prefer it.

/**
* Expected to be invoked by any {@code BaseNodesResponse} that implements {@code ToXContent} at the top level of the response object.
*/
protected XContentBuilder toInnerXContent(XContentBuilder builder, Params params) throws IOException {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like super methods that when they are overridden, they must be invoked. I also don't like that this method is on the base class and thus inherited by response objects that do not implement ToXContent, I think that is misleading. Lastly, I think that we should include total/successful/failed counts in the response.

My specific suggestion is to add a method to RestActions similar to RestActions#buildBroadcastShardsHeader. I think it can look something like this:

    public static <TNodesResponse extends BaseNodeResponse> void buildNodesInfoHeader(
            final XContentBuilder builder,
            final ToXContent.Params params,
            final BaseNodesResponse<TNodesResponse> response) throws IOException {
        final TNodesResponse[] responses = response.getNodes();
        final FailedNodeException[] failures = response.failures();
        final int successful = responses != null ? responses.length : 0;
        final int failed = failures != null ? responses.length : 0;
        buildNodesInfoHeader(builder, params, successful + failed, successful, failed, failures);
    }

    public static void buildNodesInfoHeader(
            final XContentBuilder builder,
            final ToXContent.Params params,
            final int total,
            final int successful,
            final int failed,
            final FailedNodeException[] failedNodeExceptions) throws IOException {
        // nocommit: add a constant to Fields
        builder.startObject("_nodes");
        builder.field(Fields.TOTAL, total);
        builder.field(Fields.SUCCESSFUL, successful);
        builder.field(Fields.FAILED, failed);
        if (failedNodeExceptions != null && failedNodeExceptions.length > 0) {
            builder.startArray(Fields.FAILURES);
            for (FailedNodeException failedNodeException : failedNodeExceptions) {
                builder.startObject();
                failedNodeException.toXContent(builder, params);
                builder.endObject();
            }
            builder.endArray();
        }
        builder.endObject();
    }

    public static <TNodesResponse extends BaseNodesResponse & ToXContent> BytesRestResponse nodesInfoResponse(
            final XContentBuilder builder,
            final ToXContent.Params request,
            final TNodesResponse response) throws IOException {
        builder.startObject();
        RestActions.buildNodesInfoHeader(builder, request, response);
        response.toXContent(builder, request);
        builder.endObject();
        return new BytesRestResponse(RestStatus.OK, builder);
    }

And then buildResponse call sites can just look like return RestActions.nodesInfoResponse(builder, request, response);

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to add the static method, but I do want to note that the method name is toInnerXContent rather than toXContent, so it's not overridden by any of the child implementations.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really like the concept of the "_nodes", but I'm not so sure about the name. Practically every ToXContent impl that I noticed had a "nodes" block. I'm a bit worried by how close those are.

How does that make you feel about _nodes? The problem is that I don't have a better name because that's the right one. :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to add the static method, but I do want to note that the method name is toInnerXContent rather than toXContent, so it's not overridden by any of the child implementations.

Sure but it can be overridden, if it is overridden it must be called, and it has to be called by the toXContent methods on the inheriting classes that do implement ToXContent The typical pattern to address this is to make toXContent final and have it delegate to an abstract doToXContent inner method that the inheriting classes must override. But the reason that I do not like that solution here is because not all of the inheriting classes will implement toXContent so I do not think this method should be on the super class at all.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does that make you feel about _nodes? The problem is that I don't have a better name because that's the right one. :)

Note that for the broadcast shards header in the response we use _shards. I think it should be the same here so _nodes?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that for the broadcast shards header in the response we use _shards. I think it should be the same here so _nodes?

Convincing enough to me. I glanced around and a few of its uses did add a "shards" field (though not always at the same level).

@jasontedor
Copy link
Member

jasontedor commented Apr 27, 2016

@pickypg I think it looks great, I left a few more comments. I asked @nik9000 to take a look at the StreamInput/StreamOutput code though.

I force pushed as it touched a lot of the existing code.

😦

/**
* Tests {@link StreamInput}.
*/
public class StreamInputTests extends ESTestCase {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We've just been adding tests for this StreamInput and StreamOutput to BytesStreamsTests.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mockito isn't really required for testing these the way we do in BytesStreamsTests. which is always a plus.

@pickypg
Copy link
Member Author

pickypg commented Apr 28, 2016

@jasontedor / @nik9000

I changed it from NodeResponse[] to List<NodeResponse> and FailedNodeException[] to List<FailedNodeException>. I also dropped my changes from StreamInput and StreamOutput (and corresponding tests), but I did need to add a <T extends Streamable> void writeStreamableList(List<T>) (technically this could be List<? extends Streamable>, but I preferred matching the Writable variant) and <T extends Streamable> List<T> readList(Supplier<T>)).

Overall, I think it's better, but it naturally touches even more files. It took forever to get through the tests. :)

@nik9000
Copy link
Member

nik9000 commented Apr 28, 2016

If you'd prefer to do <? extends Streamable> and <? extends Writeable> I'm fine with changing both. Or just doing yours with ?. I suspect the T in the writeable one is because Writeable used to have a type parameter and that bullied people into adding the T. Maybe. Anyway, there isn't a reason to have it now that I can see.

nodesStats = new ClusterStatsNodes(in);
indicesStats = ClusterStatsIndices.readIndicesStats(in);
// it may be that the master switched on us while doing the operation. In this case the status may be null.
status = in.readBoolean() ? ClusterHealthStatus.fromValue(in.readByte()) : null;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be "more normal" to declare this as Writeable and use readOptionalWriteable and writeOptionalWriteable. You've done plenty in this PR so it can wait though!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As written this isn't symmetrical with the write method. I would prefer that it be written in a symmetrical way for ease of comparison.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using the writeOptionalWriteable made it easy to make them symmetrical (had to make ClusterHealthStatus a Writeable though).

@pickypg pickypg force-pushed the feature/always-record-base-nodes-response-failures branch from ecbfe83 to 5e98a9b Compare April 28, 2016 15:43
@pickypg
Copy link
Member Author

pickypg commented Apr 28, 2016

@nik / @jasontedor

I changed writeStreamableList and writeList to remove T as discussed. Also made other recommended changes.

@nik9000
Copy link
Member

nik9000 commented May 2, 2016

I changed writeStreamableList and writeList to remove T as discussed. Also made other recommended changes.

Cool! I spent all morning sending emails and stuff so I'm itching to write code. I'll leave this open and have another look soonish. If @jasontedor gets to it before me fine by me!

nodes = in.readList(ClusterStatsNodeResponse::readNodeResponse);

// built from nodes rather than from the stream directly
nodesStats = new ClusterStatsNodes(nodes);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do these things need to be Writeable any more?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope. Removed it from both (worse: one was Streamable and one was Writeable)!

@nik9000
Copy link
Member

nik9000 commented May 4, 2016

I left a few comments - mostly small things that have no right to block merging. My only concern is around whether or not we need the new listener - essentially should we just move all of that header logic into the toXContent methods of the BaseNodeResponse?

@jasontedor
Copy link
Member

My only concern is around whether or not we need the new listener - essentially should we just move all of that header logic into the toXContent methods of the BaseNodeResponse?

BaseNodeResponse doesn't have a toXContent method nor should it because it would be a lie because not all of the response objects that inherit from implement ToXContent.

@nik9000
Copy link
Member

nik9000 commented May 4, 2016

BaseNodeResponse doesn't have a toXContent method nor should it because it would be a lie because not all of the response objects that inherit from implement ToXContent.

Makes sense to me then.

I'm happy with it.

@pickypg pickypg force-pushed the feature/always-record-base-nodes-response-failures branch from 760cd63 to b5b04ed Compare May 5, 2016 20:47
@pickypg
Copy link
Member Author

pickypg commented May 5, 2016

@nik9000 Added it now that local tests have finished. I like having the nodes read/written from an abstract method. It also removes a bunch of boilerplate from the child classes.

@nik9000
Copy link
Member

nik9000 commented May 6, 2016

LGTM. I know there is still discussion but I don't think it is the kind of discussion that blocks merging?

@jasontedor
Copy link
Member

LGTM.

@pickypg pickypg force-pushed the feature/always-record-base-nodes-response-failures branch from b5b04ed to 11f0701 Compare May 6, 2016 18:58
Most of the current implementations of BaseNodesResponse (plural Nodes) ignore FailedNodeExceptions.

- This adds a helper function to do the grouping to TransportNodesAction
- Requires a non-null array of FailedNodeExceptions within the BaseNodesResponse constructor
- Reads/writes the array to output
- Also adds StreamInput and StreamOutput methods for generically reading and writing arrays
@pickypg pickypg force-pushed the feature/always-record-base-nodes-response-failures branch from 11f0701 to 5be79ed Compare May 6, 2016 18:59
@pickypg pickypg merged commit 5be79ed into elastic:master May 6, 2016
@pickypg pickypg deleted the feature/always-record-base-nodes-response-failures branch May 6, 2016 19:00
imotov added a commit that referenced this pull request Jul 26, 2016
This test fails because of an unknown exceptions in FsService.stats() method, which causes no stats to be returned. With this change the exception that is causing this issue is going to be logged.

Related to #19591 and #17964
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Data Management/Stats Statistics tracking and retrieval APIs >enhancement v5.0.0-alpha3
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants