HBASE-24620 : Add a ClusterManager which submits command to ZooKeeper and its Agent which picks and execute those Commands. #2299
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
Contributor
|
@lokiore - Tests failures doesn't look related but still good to confirm if these tests fail in local as well without this patch. |
Contributor
|
Let me re-build. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
mnpoonia
approved these changes
Sep 21, 2020
Contributor
|
@lokiore Can you also add your design doc under |
Contributor
Author
@virajjasani I have added Design Doc as PDF as requested. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
virajjasani
reviewed
Nov 24, 2020
Contributor
virajjasani
left a comment
virajjasani
left a comment
There was a problem hiding this comment.
Left some comments. Nice work overall !
bin/chaos-daemon.sh
Outdated
|
|
||
| # if no args specified, show usage | ||
| if [ $# -le 1 ]; then | ||
| echo $usage |
Comment on lines
37
to
46
| check_before_start(){ | ||
| #ckeck if the process is not running | ||
| mkdir -p "$HBASE_PID_DIR" | ||
| if [ -f $CHAOS_PID ]; then | ||
| if kill -0 `cat $CHAOS_PID` > /dev/null 2>&1; then | ||
| echo $command running as process `cat $CHAOS_PID`. Stop it first. | ||
| exit 1 | ||
| fi | ||
| fi | ||
| } |
Contributor
There was a problem hiding this comment.
nit: will this work?
check_before_start(){
#ckeck if the process is not running
mkdir -p "$HBASE_PID_DIR"
if [ -f "$CHAOS_PID" ]; then
if kill -0 "$(cat "$CHAOS_PID")" > /dev/null 2>&1; then
echo "$command" running as process "$(cat "$CHAOS_PID")". Stop it first.
exit 1
fi
fi
}
bin/chaos-daemon.sh
Outdated
| } | ||
|
|
||
| bin=`dirname "${BASH_SOURCE-$0}"` | ||
| bin=`cd "$bin">/dev/null; pwd` |
Contributor
There was a problem hiding this comment.
nit: bin=$(cd "$bin">/dev/null || exit; pwd)
|
|
||
| /*** | ||
| * An agent for executing destructive actions for ChaosMonkey. | ||
| * Uses ZooKeeper Watchersc and LocalShell, to do the killing |
| @InterfaceAudience.Private | ||
| public class ChaosAgent implements Watcher, Closeable, Runnable { | ||
|
|
||
| private static final Logger LOG = LoggerFactory.getLogger(ChaosAgent.class.getName()); |
Contributor
There was a problem hiding this comment.
nit: LoggerFactory.getLogger(ChaosAgent.class) should be enough.
| try { | ||
| createNewZKConnection(); | ||
| } catch (IOException e) { | ||
| LOG.error("Error creating new ZK COnnection for agent: " + e); |
Contributor
There was a problem hiding this comment.
same as others: log should have Exception as arg
| try { | ||
| zk.close(); | ||
| } catch (InterruptedException e) { | ||
| LOG.error("Error closing ZK connection : " + e); |
Contributor
There was a problem hiding this comment.
same as others: log should have Exception as arg
| } | ||
| }; | ||
|
|
||
| AsyncCallback.StatCallback setStatusWatchCallback = new AsyncCallback.StatCallback() { |
Contributor
There was a problem hiding this comment.
nit: use lambda similar to setStatusOfTaskZNodeCallback?
| } | ||
| }; | ||
|
|
||
| AsyncCallback.StringCallback submitTaskCallback = new AsyncCallback.StringCallback() { |
Contributor
There was a problem hiding this comment.
nit: use lambda similar to setStatusOfTaskZNodeCallback?
| try { | ||
| this.createNewZKConnection(); | ||
| } catch (IOException e) { | ||
| LOG.error("Error creating ZooKeeper Connection: " + e); |
Contributor
There was a problem hiding this comment.
Exception as last argument
added 2 commits
December 13, 2020 02:14
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
virajjasani
approved these changes
Dec 18, 2020
Contributor
virajjasani
left a comment
virajjasani
left a comment
There was a problem hiding this comment.
+1, left one comment, rest can be addressed separately. Thanks @lokiore
Comment on lines
+80
to
+83
| Thread t = new Thread(new ChaosAgent(conf, | ||
| ChaosUtils.getZKQuorum(conf), ChaosUtils.getHostName())); | ||
| t.start(); | ||
| t.join(); |
Contributor
There was a problem hiding this comment.
Sure, this can be follow up.
| List<Thread> tasksList = new ArrayList<>(); | ||
| for (String task : children) { | ||
| String threadName = agentName + "_" + task; | ||
| Thread t = new Thread(() -> { |
Contributor
There was a problem hiding this comment.
This can also be follow up.
Comment on lines
135
to
136
| assert (ChaosConstants.CHAOS_AGENT_STATUS_PERSISTENT_ZNODE + | ||
| ChaosConstants.ZNODE_PATH_SEPARATOR + agentName).equals(watchedEvent.getPath()); |
Contributor
There was a problem hiding this comment.
@lokiore I this here we can provide better Exception than assert. Something like:
if (!(ChaosConstants.CHAOS_AGENT_STATUS_PERSISTENT_ZNODE +
ChaosConstants.ZNODE_PATH_SEPARATOR + agentName).equals(watchedEvent.getPath()) {
throw new RuntimeException(xxx); // or something better
}
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
virajjasani
pushed a commit
that referenced
this pull request
Dec 21, 2020
… and its Agent which picks and execute those Commands (#2299) Signed-off-by: Aman Poonia <apoonia@salesforce.com> Signed-off-by: Viraj Jasani <vjasani@apache.org>
virajjasani
pushed a commit
that referenced
this pull request
Dec 21, 2020
… and its Agent which picks and execute those Commands (#2299) Signed-off-by: Aman Poonia <apoonia@salesforce.com> Signed-off-by: Viraj Jasani <vjasani@apache.org>
virajjasani
pushed a commit
that referenced
this pull request
Dec 21, 2020
… and its Agent which picks and execute those Commands (#2299) Signed-off-by: Aman Poonia <apoonia@salesforce.com> Signed-off-by: Viraj Jasani <vjasani@apache.org>
virajjasani
pushed a commit
that referenced
this pull request
Dec 21, 2020
… and its Agent which picks and execute those Commands (#2299) Co-authored-by: Viraj Jasani <vjasani@apache.org> Signed-off-by: Aman Poonia <apoonia@salesforce.com> Signed-off-by: Viraj Jasani <vjasani@apache.org>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.