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

[RHDM-348] - Add Decision central clustering configuration to jboss-k… #17

Merged
merged 1 commit into from Apr 5, 2018

Conversation

spolti
Copy link
Member

@spolti spolti commented Apr 3, 2018

…ie-modules

Thanks for submitting your Pull Request!

Please make sure your PR meets the following requirements:

  • Pull Request title is properly formatted: [CLOUD-XYA] Subject
  • Pull Request contains link to the JIRA issue
  • Pull Request contains description of the issue
  • Pull Request does not include fixes for issues other than the main ticket
  • Attached commits represent units of work and are properly formatted
  • You have read and agreed to the Developer Certificate of Origin (DCO) (see CONTRIBUTING.md)
  • Every commit contains Signed-off-by: Your Name <yourname@example.com> - use git commit -s

Copy link
Collaborator

@errantepiphany errantepiphany left a comment

Choose a reason for hiding this comment

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

Made a couple comments.

@@ -119,3 +127,33 @@ function configure_misc_security() {
function configure_metaspace() {
export GC_MAX_METASPACE_SIZE=${WORKBENCH_MAX_METASPACE_SIZE:-1024}
}

# required envs for HA
function haEnabled() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I only see this function used in one place. Is it being called from other scripts as well? If not, I think it should go in the configureHA function.

[ -n "$APPFORMER_ELASTIC_HOST" -a -n "$APPFORMER_JMS_BROKER_USER" -a -n "$APPFORMER_JMS_BROKER_PASSWORD" -a -n "$APPFORMER_JMS_BROKER_ADDRESS" ]
}

function configureHA() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can this function be renamed to configure_ha? All our other functions use under_scores vs. camelCase. So, this would be for consistency.

unset APPFORMER_ELASTIC_PORT
unset APPFORMER_ELASTIC_RETRIES
unset APPFORMER_JMS_BROKER_PASSWORD
unset APPFORMER_JMS_BROKER_USER
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't be here also APPFORMER_JMS_BROKER_ADDRESS and APPFORMTER_JMS_BROKER_PORT ?

@spolti spolti force-pushed the RHDM-348 branch 2 times, most recently from c3e21bf to ab6b9b7 Compare April 4, 2018 21:07
@spolti
Copy link
Member Author

spolti commented Apr 4, 2018

@errantepiphany I think you missed to add the comment, I can't see where you requested changes.

@sutaakar
Copy link
Contributor

sutaakar commented Apr 5, 2018

@spolti One more idea, the variable for elasticsearch address and JMS broker address seems to me inconsistent. Elasticsearch ends on "_HOST", JMS broker ends on "_ADDRESS".
Or is there any specific reason for different naming?

if [ -n "${OPENSHIFT_DNS_PING_SERVICE_NAME}" -a "${OPENSHIFT_DNS_PING_SERVICE_PORT}" ]; then
#local artemisAddress=`hostname -i`
log_info "OpenShift DNS_PING protocol envs set, verifying other needed envs for HA setup. Using ${JGROUPS_PING_PROTOCOL}"
if haEnabled ; then
Copy link
Collaborator

Choose a reason for hiding this comment

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

Curious why we still have a (now nested) haEnabled() function above? Why not just replace this line:

if haEnabled ; then

with this line?:

if [ -n "$APPFORMER_ELASTIC_HOST" -a -n "$APPFORMER_JMS_BROKER_USER" -a -n "$APPFORMER_JMS_BROKER_PASSWORD" -a -n "$APPFORMER_JMS_BROKER_ADDRESS" ] ; then

@errantepiphany errantepiphany merged commit c2e4806 into jboss-container-images:master Apr 5, 2018
@spolti spolti deleted the RHDM-348 branch June 23, 2023 18:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants