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.

Sign up for GitHub

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

Jump to bottom

ExecuteScriptRequest/ThreadDumpRequest error reporting #12437

Merged
merged 4 commits into from Mar 12, 2018
Merged

ExecuteScriptRequest/ThreadDumpRequest error reporting #12437

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2625dbe
ExecuteScriptRequest/ThreadDumpRequest error reporting
Feb 26, 2018
1f67f86
Add scriptResult to ExecuteScriptRequest's response
Mar 2, 2018
0a63419
address Jan's comment
Mar 5, 2018
8d3d892
Address Vassilis' comments
Mar 12, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
28 changes: 15 additions & 13 deletions hazelcast/src/main/java/com/hazelcast/internal/management/ManagementCenterService.java
View file
Open in desktop
Expand Up @@ -54,6 +54,7 @@
import com.hazelcast.nio.Address;
import com.hazelcast.nio.IOUtil;
import com.hazelcast.spi.ExecutionService;
import com.hazelcast.spi.InternalCompletableFuture;
import com.hazelcast.spi.Operation;
import com.hazelcast.spi.OperationService;
import com.hazelcast.util.Clock;
Expand Down Expand Up @@ -247,24 +248,26 @@ private void interruptThread(Thread thread) {
}
}

public Object callOnAddress(Address address, Operation operation) {
public InternalCompletableFuture<Object> callOnAddress(Address address, Operation operation) {
// TODO: why are we always executing on the MapService?
OperationService operationService = instance.node.nodeEngine.getOperationService();
Future future = operationService.invokeOnTarget(MapService.SERVICE_NAME, operation, address);
try {
return future.get();
} catch (Throwable t) {
return ExceptionUtil.toString(t);
}
return operationService.invokeOnTarget(MapService.SERVICE_NAME, operation, address);
}

public Object callOnThis(Operation operation) {
public InternalCompletableFuture<Object> callOnThis(Operation operation) {
return callOnAddress(instance.node.getThisAddress(), operation);
}

public Object callOnMember(Member member, Operation operation) {
Address address = member.getAddress();
return callOnAddress(address, operation);
public InternalCompletableFuture<Object> callOnMember(Member member, Operation operation) {
return callOnAddress(member.getAddress(), operation);
}

public static Object resolveFuture(Future<Object> future) {
try {
return future.get();
} catch (Throwable t) {
return ExceptionUtil.toString(t);
}
}

public void send(Address address, Operation operation) {
Expand Down Expand Up @@ -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)));
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

}
} catch (Exception e) {
logger.warning("Web server url cannot be send to the newly joined member", e);
Expand Down
43 changes: 9 additions & 34 deletions ...st/src/main/java/com/hazelcast/internal/management/operation/ScriptExecutorOperation.java
View file
Open in desktop
Expand Up @@ -16,18 +16,18 @@

package com.hazelcast.internal.management.operation;

import com.hazelcast.core.HazelcastException;
import com.hazelcast.internal.management.ManagementDataSerializerHook;
import com.hazelcast.internal.management.ScriptEngineManagerContext;
import com.hazelcast.nio.ObjectDataInput;
import com.hazelcast.nio.ObjectDataOutput;
import com.hazelcast.util.ExceptionUtil;

import java.io.IOException;

import javax.script.ScriptEngine;
import javax.script.ScriptEngineManager;
import javax.script.ScriptException;
import java.io.IOException;
import java.util.HashMap;
import java.util.Map;
import java.util.Set;

/**
* Operation to execute script on the node.
Expand All @@ -36,37 +36,29 @@ public class ScriptExecutorOperation extends AbstractManagementOperation {

private String engineName;
private String script;
private Map<String, Object> bindings;
private Object result;

@SuppressWarnings("unused")
public ScriptExecutorOperation() {
}

public ScriptExecutorOperation(String engineName, String script, Map<String, Object> bindings) {
public ScriptExecutorOperation(String engineName, String script) {
this.engineName = engineName;
this.script = script;
this.bindings = bindings;
}

@Override
public void run() throws Exception {
public void run() {
ScriptEngineManager scriptEngineManager = ScriptEngineManagerContext.getScriptEngineManager();
ScriptEngine engine = scriptEngineManager.getEngineByName(engineName);
if (engine == null) {
throw new IllegalArgumentException("Could not find ScriptEngine named '" + engineName + "'.");
}
engine.put("hazelcast", getNodeEngine().getHazelcastInstance());
if (bindings != null) {
Set<Map.Entry<String, Object>> entries = bindings.entrySet();
for (Map.Entry<String, Object> entry : entries) {
engine.put(entry.getKey(), entry.getValue());
}
}
try {
this.result = engine.eval(script);
} catch (ScriptException e) {
this.result = e.getMessage();
throw new HazelcastException(ExceptionUtil.toString(e));
}
}

Expand All @@ -79,31 +71,14 @@ public Object getResponse() {
protected void writeInternal(ObjectDataOutput out) throws IOException {
out.writeUTF(engineName);
out.writeUTF(script);
if (bindings != null) {
out.writeInt(bindings.size());
Set<Map.Entry<String, Object>> entries = bindings.entrySet();
for (Map.Entry<String, Object> entry : entries) {
out.writeUTF(entry.getKey());
out.writeObject(entry.getValue());
}
} else {
out.writeInt(0);
}
// kept for compatibility
out.writeInt(0);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

}

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

Choose a reason for hiding this comment

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

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.

int size = in.readInt();
if (size > 0) {
bindings = new HashMap<String, Object>(size);
for (int i = 0; i < size; i++) {
String key = in.readUTF();
Object value = in.readObject();
bindings.put(key, value);
}
}
}

@Override
Expand Down
6 changes: 4 additions & 2 deletions hazelcast/src/main/java/com/hazelcast/internal/management/request/ChangeWanStateRequest.java
View file
Open in desktop
Expand Up @@ -20,6 +20,7 @@
import com.hazelcast.internal.management.ManagementCenterService;
import com.hazelcast.internal.management.operation.ChangeWanStateOperation;

import static com.hazelcast.internal.management.ManagementCenterService.resolveFuture;
import static com.hazelcast.util.JsonUtil.getBoolean;
import static com.hazelcast.util.JsonUtil.getString;

Expand Down Expand Up @@ -52,8 +53,9 @@ public int getType() {
}

@Override
public void writeResponse(ManagementCenterService mcs, JsonObject out) throws Exception {
Object operationResult = mcs.callOnThis(new ChangeWanStateOperation(schemeName, publisherName, start));
public void writeResponse(ManagementCenterService mcs, JsonObject out) {
Object operationResult = resolveFuture(
mcs.callOnThis(new ChangeWanStateOperation(schemeName, publisherName, start)));
JsonObject result = new JsonObject();
if (operationResult == null) {
result.add("result", SUCCESS);
Expand Down
5 changes: 3 additions & 2 deletions hazelcast/src/main/java/com/hazelcast/internal/management/request/ClearWanQueuesRequest.java
View file
Open in desktop
Expand Up @@ -20,6 +20,7 @@
import com.hazelcast.internal.management.ManagementCenterService;
import com.hazelcast.internal.management.operation.ClearWanQueuesOperation;

import static com.hazelcast.internal.management.ManagementCenterService.resolveFuture;
import static com.hazelcast.util.JsonUtil.getString;

/**
Expand Down Expand Up @@ -49,9 +50,9 @@ public int getType() {
}

@Override
public void writeResponse(ManagementCenterService mcs, JsonObject out) throws Exception {
public void writeResponse(ManagementCenterService mcs, JsonObject out) {
ClearWanQueuesOperation operation = new ClearWanQueuesOperation(schemeName, publisherName);
Object operationResult = mcs.callOnThis(operation);
Object operationResult = resolveFuture(mcs.callOnThis(operation));
JsonObject result = new JsonObject();
if (operationResult == null) {
result.add("result", SUCCESS);
Expand Down
124 changes: 69 additions & 55 deletions hazelcast/src/main/java/com/hazelcast/internal/management/request/ExecuteScriptRequest.java
View file
Open in desktop
Expand Up @@ -19,21 +19,21 @@
import com.eclipsesource.json.JsonArray;
import com.eclipsesource.json.JsonObject;
import com.eclipsesource.json.JsonValue;
import com.hazelcast.core.Member;
import com.hazelcast.internal.management.ManagementCenterService;
import com.hazelcast.internal.management.operation.ScriptExecutorOperation;
import com.hazelcast.nio.Address;
import com.hazelcast.util.AddressUtil;
import com.hazelcast.util.ExceptionUtil;
import com.hazelcast.util.MapUtil;

import java.util.ArrayList;
import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.concurrent.ExecutionException;
import java.util.concurrent.Future;

import static com.hazelcast.util.JsonUtil.getArray;
import static com.hazelcast.util.JsonUtil.getBoolean;
import static com.hazelcast.util.JsonUtil.getString;

/**
Expand All @@ -44,26 +44,14 @@ public class ExecuteScriptRequest implements ConsoleRequest {
private String script;
private String engine;
private Set<String> targets;
private boolean targetAllMembers;
private Map<String, Object> bindings;

public ExecuteScriptRequest() {
}

public ExecuteScriptRequest(String script, String engine, boolean targetAllMembers, Map<String, Object> bindings) {
public ExecuteScriptRequest(String script, String engine, Set<String> targets) {
this.script = script;
this.engine = engine;
this.targets = new HashSet<String>(0);
this.targetAllMembers = targetAllMembers;
this.bindings = bindings;
}

public ExecuteScriptRequest(String script, String engine, Set<String> targets, Map<String, Object> bindings) {
this.script = script;
this.targets = targets;
this.engine = engine;
this.targetAllMembers = false;
this.bindings = bindings;
}

@Override
Expand All @@ -73,47 +61,53 @@ public int getType() {

@Override
public void writeResponse(ManagementCenterService mcs, JsonObject root) throws Exception {
JsonObject jsonResult = new JsonObject();
ArrayList results;
if (targetAllMembers) {
Set<Member> members = mcs.getHazelcastInstance().getCluster().getMembers();
ArrayList<Object> list = new ArrayList<Object>(members.size());
for (Member member : members) {
list.add(mcs.callOnMember(member, new ScriptExecutorOperation(engine, script, bindings)));
}
results = list;
} else {
ArrayList<Object> list = new ArrayList<Object>(targets.size());
for (String address : targets) {
AddressUtil.AddressHolder addressHolder = AddressUtil.getAddressHolder(address);
Address targetAddress = new Address(addressHolder.getAddress(), addressHolder.getPort());
list.add(mcs.callOnAddress(targetAddress, new ScriptExecutorOperation(engine, script, bindings)));
}
results = list;
Map<Address, Future<Object>> futures = MapUtil.createHashMap(targets.size());

for (String address : targets) {
AddressUtil.AddressHolder addressHolder = AddressUtil.getAddressHolder(address);
Address targetAddress = new Address(addressHolder.getAddress(), addressHolder.getPort());
futures.put(targetAddress, mcs.callOnAddress(targetAddress, new ScriptExecutorOperation(engine, script)));
}

StringBuilder sb = new StringBuilder();
for (Object result : results) {
if (result instanceof String) {
sb.append(result);
} else if (result instanceof List) {
List list = (List) result;
for (Object o : list) {
sb.append(o).append("\n");
}
} else if (result instanceof Map) {
Map map = (Map) result;
for (Object o : map.entrySet()) {
Map.Entry entry = (Map.Entry) o;
sb.append(entry.getKey()).append("->").append(entry.getValue()).append("\n");
JsonObject responseJson = new JsonObject();
StringBuilder scriptResult = new StringBuilder();
for (Map.Entry<Address, Future<Object>> entry : futures.entrySet()) {
Address address = entry.getKey();
Future<Object> future = entry.getValue();

try {
Object result = future.get();
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

@jbee jbee Mar 2, 2018 •
edited

Choose a reason for hiding this comment

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

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".


StringBuilder sb = new StringBuilder();
if (result instanceof String) {
sb.append(result);
} else if (result instanceof List) {
List list = (List) result;
for (Object o : list) {
sb.append(o).append("\n");
}
} else if (result instanceof Map) {
Map map = (Map) result;
for (Object o : map.entrySet()) {
Map.Entry e = (Map.Entry) o;
sb.append(e.getKey()).append("->").append(e.getValue()).append("\n");
}
} else if (result == null) {
sb.append("error");
}
} else if (result == null) {
sb.append("error");
sb.append("\n");

addSuccessResponse(responseJson, scriptResult, address, sb.toString());
} catch (ExecutionException e) {
addErrorResponse(responseJson, scriptResult, address, e);
} catch (InterruptedException e) {
addErrorResponse(responseJson, scriptResult, address, e);
Thread.currentThread().interrupt();
}
sb.append("\n");
}
jsonResult.add("scriptResult", sb.toString());
root.add("result", jsonResult);

responseJson.add("scriptResult", scriptResult.toString());
root.add("result", responseJson);
}

@Override
Expand All @@ -124,7 +118,27 @@ public void fromJson(JsonObject json) {
for (JsonValue target : getArray(json, "targets", new JsonArray())) {
targets.add(target.asString());
}
targetAllMembers = getBoolean(json, "targetAllMembers", false);
bindings = new HashMap<String, Object>();
}

private static void addSuccessResponse(JsonObject root, StringBuilder scriptResult,
Address address, String result) {

addResponse(root, scriptResult, address, true, result);
}

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

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

}

private static void addResponse(JsonObject root, StringBuilder scriptResult,
Address address, boolean success, String result) {

JsonObject json = new JsonObject();
json.add("success", success);
json.add("result", result);
root.add(address.toString(), json);

scriptResult.append(result);
}
}
5 changes: 3 additions & 2 deletions hazelcast/src/main/java/com/hazelcast/internal/management/request/MapConfigRequest.java
View file
Open in desktop
Expand Up @@ -26,6 +26,7 @@

import java.util.Set;

import static com.hazelcast.internal.management.ManagementCenterService.resolveFuture;
import static com.hazelcast.util.JsonUtil.getBoolean;
import static com.hazelcast.util.JsonUtil.getObject;
import static com.hazelcast.util.JsonUtil.getString;
Expand Down Expand Up @@ -60,11 +61,11 @@ public void writeResponse(ManagementCenterService mcs, JsonObject root) {
if (update) {
final Set<Member> members = mcs.getHazelcastInstance().getCluster().getMembers();
for (Member member : members) {
mcs.callOnMember(member, new UpdateMapConfigOperation(mapName, config.getMapConfig()));
resolveFuture(mcs.callOnMember(member, new UpdateMapConfigOperation(mapName, config.getMapConfig())));
}
result.add("updateResult", "success");
} else {
MapConfig cfg = (MapConfig) mcs.callOnThis(new GetMapConfigOperation(mapName));
MapConfig cfg = (MapConfig) resolveFuture(mcs.callOnThis(new GetMapConfigOperation(mapName)));
if (cfg != null) {
result.add("hasMapConfig", true);
result.add("mapConfig", new MapConfigDTO(cfg).toJson());
Expand Down