hyperledger / besu

An enterprise-grade Java-based, Apache 2.0 licensed Ethereum client https://wiki.hyperledger.org/display/besu
https://www.hyperledger.org/projects/besu
Apache License 2.0
1.44k stars 765 forks source link

Implementing Issue #7047 - Optionally load jemalloc #7103

Closed amsmota closed 2 months ago

amsmota commented 2 months ago

PR description

Implementing a optional configuration via the existing environment variable BESU_USING_JEMALLOCto enable/disable the loading of jemalloc when starting Besu.

If set, BESU_USING_JEMALLOC=true will load jemalloc, if set to anything else it won't. If BESU_USING_JEMALLOC is not set at all it'll load jemalloc too (that is the existing behavior).

Please note the installation of jemalloc itself must be done using symlinks, otherwise the initial besu script must be changed to provide the name of the installed libjemalloc.so.

In Ubunto, for instance, I used

sudo apt-get install -y libjemalloc-dev

Fixed Issue(s)

Optionally load jemalloc #7047->

Thanks for sending a pull request! Have you done the following?

Locally, you can run these tests to catch failures early:

amsmota commented 2 months ago

so basically the user would need to set the environment variable explicitly, if they don't want jemalloc used?

Yes, a env var BESU_USING_JEMALLOC=true set at the OS or Container level.

prob needs a changelog entry

Can you tell me how can I do that?

and a suggestion for how we would doc this

I think a shorter version of this PR Description will be enough.

amsmota commented 2 months ago

Closed by mistake, unfortunately it can't be reopened because I had issues with my local branch and I had to rebase anf force push... If needed I suppose I have to create another branch?

image

amsmota commented 1 month ago

@usmansaleem, @macfarla, can youse please review/test if possible this lat version I pushed? It seems very straighforward and tbh there is only some much I can test more...

amsmota commented 1 month ago

@macfarla, can you tell me what to do for that changelog entry?

And do you agree that a shorter version of this PR Description will be enough for the documentation?

BTW, I think I misread your question and my answer is wrong, if the user DOES NOT want jemalloc used, then he MUST set BESU_USING_JEMALLOC to anything that evaluates to false in Boolean.parseBoolean(t)...

 

so basically the user would need to set the environment variable explicitly, if they don't want jemalloc used?

Yes, a env var BESU_USING_JEMALLOC=true set at the OS or Container level.

prob needs a changelog entry

Can you tell me how can I do that?

and a suggestion for how we would doc this

I think a shorter version of this PR Description will be enough.

macfarla commented 1 month ago

Closed by mistake, unfortunately it can't be reopened because I had issues with my local branch and I had to rebase anf force push... If needed I suppose I have to create another branch?

image

I don't have permissions to reopen your PR either (the branch was force-pushed or recreated), I think you should be able to open a new PR from the same branch if you can't reopen this one.

Re the changelog - add a line to CHANGELOG.md under "Additions and Improvements" with a brief description and a link to the PR eg Allow optional loading of jemalloc using an environment variable

amsmota commented 1 month ago

It just occurred to me, most of Besu configurations has a ENV format, a CLI format and a config-file. I should actually do the same with the BESU_USING_JEMALLOC env var, right? Or the fact that it is an external option prevents it? (I don't know, maybe copyrights or something like that.)

macfarla commented 1 month ago

It just occurred to me, most of Besu configurations has a ENV format, a CLI format and a config-file. I should actually do the same with the BESU_USING_JEMALLOC env var, right? Or the fact that it is an external option prevents it? (I don't know, maybe copyrights or something like that.)

I think we use environment variable because it's easier to access from the start script.

amsmota commented 1 month ago

Re the changelog - add a line to CHANGELOG.md under "Additions and Improvements" with a brief description and a link to the PR eg Allow optional loading of jemalloc using an environment variable

I just added the following to the changelog:

amsmota commented 1 month ago

I think we use environment variable because it's easier to access from the start script.

@macfarla, @usmansaleem, If that'sthe case I suggest to finish this PR and merge it. Can youse tell me what are the steps I need to do?

Thanks.