-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Add pricenode one-command installer script, systemd service, README #3997
Conversation
@wiz Please have a look at: https://app.codacy.com/gh/bisq-network/bisq/pullRequest?prid=5047646 |
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.
NACK - Please see #3997 (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.
Nit, see comments.
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.
NACK
- the config for the JVM heap monitoring has been changed so that it constantly creates errors for pricenodes (because the jmxremote-access point is not configured) AND there is not java heapspace monitoring
- see comment for line 55
eca27f7
to
c23f032
Compare
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.
see #3997 (review)
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.
I would suggest adding a /etc/default/bisq.env
file just as the seed node has with the following contents:
JAVA_OPTS=""
BITCOIN_AVG_PUBKEY=__BITCOIN_AVG_PUBKEY__
BITCOIN_AVG_PRIVKEY=__BITCOIN_AVG_PRIVKEY__
and revert the if-check in the collectd install script.
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.
the base is set now. will provide a followup-PR tweaking the monitoring stuff soon.
8816690
to
3df64b2
Compare
Thanks for the ACK, just squashed the commits |
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.
I am abstaining from any further review on this. The changes are not substantive with regard to the pricenode itself, and I'll leave it to others here to determine whether the installer works as desired.
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.
sry. Thought it is good enough, However, having a hard-coded version in the service file seems off.
What do you guys think? @cbeams?
The current pricenode version is BS in any case. It's an artifact of the pricenode having been formerly split off into a separate repository. Agree that the version should not be hard coded in the installer in any case. The version should be the same as the rest of Bisq (as it is versioned with Bisq). That change is out of scope here, just mentioning it. As far as using the `bisq-pricenode` script, I don't see any reason not to, but again, I'm not really familiar with what's going on in this PR. Would leave it to @wiz to say.
… On Feb 24, 2020, at 3:06 PM, Florian Reimair ***@***.***> wrote:
@freimair requested changes on this pull request.
sry. Thought it is good enough, However, having a hard-coded version in the service file seems off.
What do you guys think? @cbeams <https://github.com/cbeams>?
In pricenode/bisq-pricenode.service <#3997 (comment)>:
> @@ -0,0 +1,21 @@
+[Unit]
+Description=Bisq Price Node
+After=network.target
+
+[Service]
+EnvironmentFile=/etc/default/bisq-pricenode.env
+ExecStart=/usr/bin/java -jar /bisq/bisq/lib/pricenode-0.7.2-SNAPSHOT.jar 2 2
Is there a specific reason not to use the start script /bisq/bisq/bisq-pricenode for the price node?
As there is a specific version hard coded in the service file seems like we might risk missing updating the bisq.service file and thus, producing maintenance issues and efforts.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub <#3997?email_source=notifications&email_token=AACJV4THEN7CCTTMQXFF3HDREPH77A5CNFSM4KZB4VH2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCWUTJCA#pullrequestreview-363410568>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACJV4W3TD3JRYAUW63UUS3REPH77ANCNFSM4KZB4VHQ>.
|
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.
using the standard start script solves the JVM heap monitoring issues (#4004)
Co-Authored-By: Florian Reimair <[email protected]>
Co-Authored-By: Florian Reimair <[email protected]>
@cbeams Could you please resolve your change request or comment if you do want to have specific changes implemented? |
Done (approved). Thanks. |
No description provided.