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

[fix][doc] Fix M1 JVM Installation Instructions #17669

Merged
merged 2 commits into from
Sep 30, 2022

Conversation

asafm
Copy link
Contributor

@asafm asafm commented Sep 15, 2022

Modifications

Fix the documentation of installing JVM on M1 - remove one installation method of sdkman (using brew) and add the deleted instructions to building Pulsar on README

Documentation

  • doc-required
    (Your PR needs to update docs and you will update later)

  • doc-not-needed
    (Please explain why)

  • doc
    (Your PR contains doc changes)

  • doc-complete
    (Docs have been already added)

README.md Outdated
@@ -138,6 +138,47 @@ Requirements:
> Note: this project includes a [Maven Wrapper](https://maven.apache.org/wrapper/) that can be used instead of a system installed Maven.
> Use it by replacing `mvn` by `./mvnw` on Linux and `mvnw.cmd` on Windows in the commands below.

### Install JDK on M1
Copy link
Member

@tisonkun tisonkun Sep 15, 2022

Choose a reason for hiding this comment

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

I wrote:

"how to install JDK on M1" is not suitable to document on the Get Started page. Imagine that you don't tell users how to buy a computer. It's far away from Pulsar topics. Users can resolve it with Google, StackOverflow, etc.

in #17409 (comment). This stands for README.md also. I can imagine users have multiple approaches to do this and we don't have to use forty lines for it. If someone asked, we resolve the discussion or issue. And followers should be able to search or we simply answer by throwing the link.

Copy link
Member

Choose a reason for hiding this comment

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

For example, I install JDK on M1 simply with sdk install java without all the troubleshooting here and it works well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This issue happened to me and 5 other engineers I know of just as an example.
Our approach, especially on getting started should be giving you everything you need to get started - in the README case, it's building the product. M1 is notorious to have many quirks. Anything we can do to help will be greatly appreciated.

Just mentioning a few people recently running into those issues @geomagilles @teabot @nahguam just to name a few

Bear in mind, that they can ignore this recommendation. Most newcomers will appreciate it as much as I did.

@github-actions github-actions bot added the doc Your PR contains doc changes, no matter whether the changes are in markdown or code files. label Sep 15, 2022
README.md Outdated
@@ -138,6 +138,47 @@ Requirements:
> Note: this project includes a [Maven Wrapper](https://maven.apache.org/wrapper/) that can be used instead of a system installed Maven.
> Use it by replacing `mvn` by `./mvnw` on Linux and `mvnw.cmd` on Windows in the commands below.

### Install JDK on M1
In the current version, Pulsar uses a BookKeeper version which in turn uses RocksDB. RocksDB is compiled to work on x86 architecture and not ARM. Therefore, Pulsar can only work with x86 JDK. This is planned to be fixed in future versions of Pulsar.
Copy link
Member

@tisonkun tisonkun Sep 18, 2022

Choose a reason for hiding this comment

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

This doesn't stand any more on the nightly version. See also #12166 (comment).

I agree that there can be a few troubleshooting to work on a different environment with older version, and I'd prefer a discussion thread (#12166 seems a good place to comment these instructions and closed).

Copy link
Member

@tisonkun tisonkun Sep 18, 2022

Choose a reason for hiding this comment

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

And I just notice that it misleads me because it's not about "Install JDK on M1" - it's about "Install X86 JDK on M1" (if you get a RocksDB JNI load exception).

https://www.google.com/search?q=Install+x86+JDK+on+M1 gives you:

That can help.

Copy link
Member

@tisonkun tisonkun Sep 18, 2022

Choose a reason for hiding this comment

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

So the meaningful information here is, that if you fail to run Pulsar on M1 due to RocksDB JNI error (with a typical stack trace), the reason is that you need an X86 JDK for older versions. Then mention some methods to achieve it as you like.

I get confused the first time: I'm an M1 user, and this section seems written for me. But it's not. It's for those who running Pulsar <= 2.10 on M1 and get the RocksDB JNI load exception.

Copy link
Member

Choose a reason for hiding this comment

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

And perhaps after we release 2.11.0 and users start to use the new version, it will be less and less useful.

M1 is notorious to have many quirks. Anything we can do to help will be greatly appreciated.

I agree. So +0 on this patch now. FYI - I encounter several other issues, and I prefer to resolve troubleshootings in discussions instead of a manual try to cover most of them:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. I agree. It's mostly targeted at users trying to develop Pulsar and since it was solved on master, it's not beneficial anymore.

@asafm
Copy link
Contributor Author

asafm commented Sep 20, 2022

@tisonkun I reduced the PR by just fixing 2.10 instructions then

Copy link
Member

@tisonkun tisonkun left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you!

@asafm
Copy link
Contributor Author

asafm commented Sep 29, 2022

@tisonkun How do we continue from here?

@tisonkun
Copy link
Member

Let me try to find a committer to help with reviewing and merging. I don't have write permission here :)

@momo-jun @Anonymitaet @merlimat

@Anonymitaet Anonymitaet added this to the 2.11.0 milestone Sep 30, 2022
@Anonymitaet Anonymitaet merged commit 5f07943 into apache:master Sep 30, 2022
@asafm asafm deleted the fix-sdkman-brew branch October 2, 2022 08:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Your PR contains doc changes, no matter whether the changes are in markdown or code files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants