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
RATIS-1537. Support run and debug ratis server in intellij #612
Conversation
@szetszwo Can you help take a look at this MR, thx! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@codings-dan , thanks for working on this. Some comments inlined.
SRC_DIR="$SCRIPT_DIR/runConfigurations" | ||
DEST_DIR="$SCRIPT_DIR/../../.idea/runConfigurations/" | ||
mkdir -p "$DEST_DIR" | ||
ls -1 "$SRC_DIR" | xargs -n1 -I FILE cp "$SRC_DIR/FILE" "$DEST_DIR" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not simply cp -i "${SRC_DIR}"/* "${DEST_DIR}"
? Is there a reason to use ls and then xargs?
BTW, it is good to add -i
so that it will take care if the file exists.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for reviewing the code, cp -i
is indeed a better implementation.
SRC_DIR="$SCRIPT_DIR/runConfigurations" | ||
DEST_DIR="$SCRIPT_DIR/../../.idea/runConfigurations/" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's print out the dirs and show the mkdir and cp commands.
echo src : "${SRC_DIR}"
echo dest: "${DEST_DIR}"
set -ex
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
limitations under the License. See accompanying LICENSE file. | ||
--> | ||
<component name="ProjectRunConfigurationManager"> | ||
<configuration default="false" name="Server1" type="Application" factoryName="Application" nameIsGenerated="true"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's call it ExampleServer1
. Please change also the file name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
<configuration default="false" name="Server1" type="Application" factoryName="Application" nameIsGenerated="true"> | ||
<option name="MAIN_CLASS_NAME" value="org.apache.ratis.examples.debug.server.Server" /> | ||
<module name="ratis-examples" /> | ||
<option name="PROGRAM_PARAMETERS" value="127.0.0.1:10024" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see if we can easily generate this conf file from ratis-examples/src/main/resources/conf.properties
. If it is too complicated, please add a comment such as below.
<!-- See ratis-examples/src/main/resources/conf.properties -->
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't seem to be easy, so I've added a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 the change looks good.
What changes were proposed in this pull request?
we can easily run three ratis server in Intellij, and debug it.
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/RATIS-1537
How was this patch tested?
No need