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

RATIS-1581. Add ratis-shell compile and command document #646

Merged
merged 3 commits into from
May 18, 2022

Conversation

codings-dan
Copy link
Contributor

@codings-dan
Copy link
Contributor Author

@szetszwo Can you help review this patch, thanks!

Copy link
Contributor

@szetszwo szetszwo left a comment

Choose a reason for hiding this comment

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

@codings-dan , thanks a lot for working on this.

It is a good idea to have a section discussing how to setup ratis-shell. We should point the user to download the src tarball and describe how to build from it.

How about the bin tarball? How could we make ratis-shell work from the bin tarball without building it from the source?

Get the ratis-shell source and Configure environment variables

```
$ mvn -DskipTests -Prelease -Papache-release clean package assembly:single
Copy link
Contributor

Choose a reason for hiding this comment

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

We should discuss how to run it from a release. Let's ask the user to download the src tarball from https://ratis.apache.org/downloads.html and mention that ratis shell is available starting from 2.3.0.

Comment on lines 34 to 36
$ tar -C /tmp -xzf ratis-assembly/target/apache-ratis-2.3.0-SNAPSHOT-src.tar.gz

$ cd /tmp/apache-ratis-2.3.0-SNAPSHOT
Copy link
Contributor

Choose a reason for hiding this comment

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

  • /tmp seems not a good location since the files could be deleted by the system.
  • Let's do not hardcode the version number.
$ tar -C <DST_DIR> -xzf ratis-assembly/target/apache-ratis-*-src.tar.gz

$ cd <DST_DIR>/apache-ratis-<VERSION>

@@ -23,6 +23,24 @@ Ratis-shell is the command line interface of Ratis.
> Ratis-shell is currently only **experimental**.
> The compatibility story is not considered for the time being.


Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add a section title.

## Setting up ratis-shell

@codings-dan
Copy link
Contributor Author

codings-dan commented May 17, 2022

We should point the user to download the src tarball and describe how to build from it.

Sure, we could make user download the src tarball and executethe following command to build and get the ratis-shell
mvn -DskipTests -Prelease -Papache-release clean package assembly:single

How about the bin tarball? How could we make ratis-shell work from the bin tarball without building it from the source?

Good idea! User can get the ratis-shell from bin tarball without building it. Am I not authorized to change the contents of the bin tarball right now? Is there anything that I can do to support user getting ratis-shell directly from the bin tarball?

Copy link
Contributor

@szetszwo szetszwo left a comment

Choose a reason for hiding this comment

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

@codings-dan , thanks a lot for the update! Some comments inlined. See also https://issues.apache.org/jira/secure/attachment/13043803/646_review.patch

@@ -23,6 +23,37 @@ Ratis-shell is the command line interface of Ratis.
> Ratis-shell is currently only **experimental**.
> The compatibility story is not considered for the time being.


## Get the ratis-shell source
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this section is quite short. Let's call it "Setting up the ratis-shell" for including the entire setup and don't have any subsections.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

...
[INFO] BUILD SUCCESS

$ tar -C <DST_DIR>/ratis-shell -xzf ratis-assembly/target/apache-ratis-2.3.0-SNAPSHOT-src.tar.gz
Copy link
Contributor

Choose a reason for hiding this comment

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

It should use the shell.tar.gz but not src.tar.gz.

$ tar -C <DST_DIR>/ratis-shell -xzf ratis-assembly/target/apache-ratis-<VERSION>-shell.tar.gz

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done


$ tar -C <DST_DIR>/ratis-shell -xzf ratis-assembly/target/apache-ratis-2.3.0-SNAPSHOT-src.tar.gz

$ cd <DST_DIR>/ratis-shell/apache-ratis-2.3.0-SNAPSHOT
Copy link
Contributor

@szetszwo szetszwo May 17, 2022

Choose a reason for hiding this comment

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

Replace "2.3.0-SNAPSHOT" with <VERSION>.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done


$ cd <DST_DIR>/ratis-shell/apache-ratis-2.3.0-SNAPSHOT
```
TODO(codings-dan): Get `ratis-shell` from bin tarball directly
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's remove this TODO. Otherwise, it will show up in the doc.

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 have remove this TODO. I will add this part in the future. What can I do to make user get ratis-shell from bin tarball directly, I'd love to take this task and complete it!

@codings-dan
Copy link
Contributor Author

@szetszwo Thanks for helping change the document, PTAL again, thanks!

Copy link
Contributor

@szetszwo szetszwo left a 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.

@szetszwo szetszwo merged commit fc61c29 into apache:master May 18, 2022
@codings-dan codings-dan deleted the 1581 branch May 18, 2022 13:29
@codings-dan
Copy link
Contributor Author

Thanks @szetszwo for reviewing the code!

symious pushed a commit to symious/ratis that referenced this pull request Mar 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants