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

ExecuteScriptRequest/ThreadDumpRequest error reporting #12437

Merged

Conversation

@emre-aydin
Copy link
Member

@emre-aydin emre-aydin commented Feb 27, 2018

These two types of requests are sent from MC to members. When they
finish, they don't make any difference between an error result and
a success result. With this fix, a distinction is made so that MC can
log the error in its logs and show the success result on the UI.

ExecuteScriptRequest is changed so that it returns responses from each
member in a separate JSON field. MC can report error/success
from each member separately as a result.

Unused fields are removed from ScriptExecutorOperation, while
taking care to preserve its compatibility with previous member versions.

PR on Management Center

@emre-aydin emre-aydin added this to the 3.9.4 milestone Feb 27, 2018
@emre-aydin emre-aydin self-assigned this Feb 27, 2018
@emre-aydin
Copy link
Member Author

@emre-aydin emre-aydin commented Feb 27, 2018

verify

These two classes don't make any difference between an error result and
a success result. With this fix, a distinction is made so that MC can
log the error in its logs and show the success result on the UI.

ExecuteScriptRequest is changed so that it returns responses from each
member in a separate JSON field so that MC can report error/success
from each member separately.

Also, unused fields are removed from ScriptExecutorOperation, while
taking care to preserve its compatibility with previous member versions.
In the forward port of this change, we can it into a version aware
operation.
@emre-aydin emre-aydin force-pushed the emre-aydin:execute-script-error-reporting branch from 0be92d5 to 2625dbe Feb 27, 2018
out.writeInt(0);
}
// kept for compatibility
out.writeInt(0);

This comment has been minimized.

@emre-aydin

emre-aydin Feb 27, 2018
Author Member

Will be surrounded by a version check and conditional serialization in 3.10 with the forward port of this PR.

@devOpsHazelcast
Copy link
Contributor

@devOpsHazelcast devOpsHazelcast commented Feb 27, 2018

Test PASSed.

@emre-aydin
Copy link
Member Author

@emre-aydin emre-aydin commented Feb 27, 2018

verify

@devOpsHazelcast
Copy link
Contributor

@devOpsHazelcast devOpsHazelcast commented Feb 27, 2018

Test PASSed.

@emre-aydin emre-aydin changed the title [WIP] ExecuteScriptRequest/ThreadDumpRequest error reporting ExecuteScriptRequest/ThreadDumpRequest error reporting Feb 28, 2018
@emre-aydin emre-aydin requested review from jbee and vbekiaris Feb 28, 2018
It's needed for compatibility with older MC versions
Copy link
Contributor

@jbee jbee left a comment

Some code style issues that should be changed or discussed.

Future<Object> future = entry.getValue();

try {
Object result = future.get();

This comment has been minimized.

@jbee

jbee Mar 2, 2018
Contributor

I think the threading/async resolving and its error handling should not be intertwined with composing a script String.

This comment has been minimized.

@emre-aydin

emre-aydin Mar 2, 2018
Author Member

I went back and forth between creating a custom class for returning the call's success/failure output together with its result/error message (since Java doesn't allow multiple return values from a method) and leaving it to Future handling code. In the end, I chose to go with leaving it to Future handling code. Just to make it clear, you think the ManagementCenterService method should resolve the Future and return the status and the message to its callers?

This comment has been minimized.

@jbee

jbee Mar 2, 2018
Contributor

Not necessarily - I just mean composing a string and error handling required because of future.get() are two things and better done one after another. Think about the changes need if you would want to test just the string composition from "data".

result.add("dump", threadDump);
} else {
result.add("hasDump", false);
InternalCompletableFuture<Object> future = mcs.callOnThis(new ThreadDumpOperation(dumpDeadlocks));

This comment has been minimized.

@jbee

jbee Mar 2, 2018
Contributor

Similarly, why do we deal with the future here and not earlier?

JsonObject json = getObject(result, nodeAddressWithBrackets);
assertTrue(getBoolean(json, "success"));
assertEquals("error\n", getString(json, "result"));
assertEquals("error\n", getString(result, "scriptResult"));

This comment has been minimized.

@jbee

jbee Mar 2, 2018
Contributor

This case seams inconsistent with the others by having an \n at the end.

This comment has been minimized.

@emre-aydin

emre-aydin Mar 2, 2018
Author Member

Couldn't understand this one. Other tests in this class don't compare the output messages as a whole, they look for substrings or check if an empty response is returned.

This comment has been minimized.

@jbee

jbee Mar 2, 2018
Contributor

I just mean that this response ends with a \n while the other ones don't. Why would just the "error" message end with \n? It feels odd.

This comment has been minimized.

@emre-aydin

emre-aydin Mar 5, 2018
Author Member

I guess it was done so because the original developers preferred to display the message on MC as is , so they have added the newline to the message on the cluster side. For example, here we also see the newlines being added to the messages' end:

@emre-aydin
Copy link
Member Author

@emre-aydin emre-aydin commented Mar 5, 2018

verify

@jbee
jbee approved these changes Mar 5, 2018
@devOpsHazelcast
Copy link
Contributor

@devOpsHazelcast devOpsHazelcast commented Mar 5, 2018

Test PASSed.

out.writeInt(0);
}
// kept for compatibility
out.writeInt(0);
}

@Override
protected void readInternal(ObjectDataInput in) throws IOException {
engineName = in.readUTF();
script = in.readUTF();

This comment has been minimized.

@vbekiaris

vbekiaris Mar 9, 2018
Contributor

Here we still have to consume the int that was written from writeInternal even if it's not used any more. Otherwise the next packet will be misaligned and serialization exceptions will occur.

@@ -637,8 +640,7 @@ public void memberAdded(MembershipEvent membershipEvent) {
try {
Member member = membershipEvent.getMember();
if (member != null && instance.node.isMaster() && urlChanged) {
Operation operation = new UpdateManagementCenterUrlOperation(managementCenterUrl);
callOnMember(member, operation);
resolveFuture(callOnMember(member, new UpdateManagementCenterUrlOperation(managementCenterUrl)));

This comment has been minimized.

@vbekiaris

vbekiaris Mar 9, 2018
Contributor

I 'd rather keep the Operation construction on a separate line for readability.


private static void addErrorResponse(JsonObject root, StringBuilder scriptResult,
Address address, Exception e) {
addResponse(root, scriptResult, address, false, ExceptionUtil.toString(e));

This comment has been minimized.

@vbekiaris

vbekiaris Mar 9, 2018
Contributor

Do we have to append all the exception results to the scriptResult field? Since the scriptResult is kept for compatibility and the goal is to avoid displaying exceptions on the UI, could we skip appending exceptions, so even older mancenter versions will not display stack traces? Another option could be to just append the exception message instead of the whole stack trace: this way we still indicate an error occurred and avoid showing stack traces. wdyt?

This comment has been minimized.

@emre-aydin

emre-aydin Mar 12, 2018
Author Member

the goal is to avoid displaying exceptions on the UI

I think we should still have the full stacktrace, otherwise we wouldn't be able to show the exception details in logs either (which would limit usability of the scripting tab). As this is a security fix, we can always ask the user to upgrade to latest to have full security.

I have addressed all other comments. Thanks for the review.

@emre-aydin emre-aydin merged commit c40e642 into hazelcast:maintenance-3.x Mar 12, 2018
1 check passed
1 check passed
default Test PASSed.
Details
@emre-aydin emre-aydin deleted the emre-aydin:execute-script-error-reporting branch Mar 12, 2018
emre-aydin added a commit to emre-aydin/hazelcast that referenced this pull request Mar 14, 2018
* ExecuteScriptRequest/ThreadDumpRequest error reporting

These two classes don't make any difference between an error result and
a success result. With this fix, a distinction is made so that MC can
log the error in its logs and show the success result on the UI.

ExecuteScriptRequest is changed so that it returns responses from each
member in a separate JSON field so that MC can report error/success
from each member separately.

Also, unused fields are removed from ScriptExecutorOperation, while
taking care to preserve its compatibility with previous member versions.
In the forward port of this change, we can it into a version aware
operation.

* Add scriptResult to ExecuteScriptRequest's response

It's needed for compatibility with older MC versions

(cherry picked from commit c40e642)
emre-aydin added a commit to emre-aydin/hazelcast that referenced this pull request Mar 20, 2018
* ExecuteScriptRequest/ThreadDumpRequest error reporting

These two classes don't make any difference between an error result and
a success result. With this fix, a distinction is made so that MC can
log the error in its logs and show the success result on the UI.

ExecuteScriptRequest is changed so that it returns responses from each
member in a separate JSON field so that MC can report error/success
from each member separately.

Also, unused fields are removed from ScriptExecutorOperation, while
taking care to preserve its compatibility with previous member versions.
In the forward port of this change, we can it into a version aware
operation.

* Add scriptResult to ExecuteScriptRequest's response

It's needed for compatibility with older MC versions

(cherry picked from commit c40e642)
emre-aydin added a commit to emre-aydin/hazelcast that referenced this pull request Mar 20, 2018
* ExecuteScriptRequest/ThreadDumpRequest error reporting

These two classes don't make any difference between an error result and
a success result. With this fix, a distinction is made so that MC can
log the error in its logs and show the success result on the UI.

ExecuteScriptRequest is changed so that it returns responses from each
member in a separate JSON field so that MC can report error/success
from each member separately.

Also, unused fields are removed from ScriptExecutorOperation, while
taking care to preserve its compatibility with previous member versions.
In the forward port of this change, we can it into a version aware
operation.

* Add scriptResult to ExecuteScriptRequest's response

It's needed for compatibility with older MC versions

(cherry picked from commit c40e642)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.