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.

Sign up for GitHub

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

Jump to bottom

Allow absolute paths in HAZELCAST_CONFIG environment variable #19908

Merged
merged 5 commits into from
Nov 9, 2021
Merged

Allow absolute paths in HAZELCAST_CONFIG environment variable #19908

merged 5 commits into from
Nov 9, 2021

Conversation

yuce
Copy link
Contributor

@yuce yuce commented Nov 8, 2021

This PR updates bin/hz-start script to allow absolute paths in HAZELCAST_CONFIG. The following rules are used to determine the actual configuration path:

  • HAZELCAST_CONFIG is empty or not set: $HAZELCAST_HOME/config/hazelcast.xml
  • HAZELCAST_CONFIG is absolute (starts with /): The configuration path is used as is.
  • Otherwise $HAZELCAST_HOME/$HAZELCAST_CONFIG

Checklist:

  • Labels (Team:, Type:, Source:, Module:) and Milestone set
  • Label Add to Release Notes or Not Release Notes content set
  • Request reviewers if possible
  • Send backports/forwardports if fix needs to be applied to past/future releases
  • New public APIs have @Nonnull/@Nullable annotations
  • New public APIs have @since tags in Javadoc

ldziedziul
ldziedziul reviewed Nov 8, 2021
View reviewed changes
distribution/src/bin-filemode-755/hz-start Outdated
if [ -z "$HAZELCAST_CONFIG" ]; then
HAZELCAST_CONFIG="config/hazelcast.xml"
HAZELCAST_CONFIG="$HAZELCAST_HOME/config/hazelcast.xml"
else
Copy link
Contributor

Choose a reason for hiding this comment

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

You can avoid nesting conditions: in the first if you can leave the code as it was and simply add another if below:

if [ -z "$HAZELCAST_CONFIG" ]; then
    HAZELCAST_CONFIG="config/hazelcast.xml"
fi

# if the first character is /, then this is an absolute path, use as is
# otherwise prepend (HAZELCAST_HOME)
if [ "${HAZELCAST_CONFIG:0:1}" != "/" ]; then
  HAZELCAST_CONFIG="$HAZELCAST_HOME/$HAZELCAST_CONFIG"
fi

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense. Updated at: cda27a4

ldziedziul
ldziedziul reviewed Nov 8, 2021
View reviewed changes
distribution/src/bin-filemode-755/hz-start Outdated
if [ -z "$HAZELCAST_CONFIG" ]; then
HAZELCAST_CONFIG="config/hazelcast.xml"
HAZELCAST_CONFIG="$HAZELCAST_HOME/config/hazelcast.xml"
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't have to duplicate here how the HAZELCAST_CONFIG is resolved. HAZELCAST_HOME will be added by the condition below :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, fixed at: 8e5e8f0

@hz-devops-test
Copy link

The job Hazelcast-pr-builder of your PR failed. (Hazelcast internal details: build log, artifacts).
Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log file 
--------------------------
-------TEST FAILURE-------
--------------------------
[INFO] Results:
[INFO] 
[ERROR] Failures: 
[ERROR]   QueueStatisticsTest.testAge:170 expected:<4611686018427387903> but was:<0>
[ERROR]   QueueStatisticsTest.testAge:170 expected:<4611686018427387903> but was:<0>
[INFO] 
[ERROR] Tests run: 46513, Failures: 2, Errors: 0, Skipped: 1012
[INFO] 

[ERROR] There are test failures.

ldziedziul
ldziedziul approved these changes Nov 8, 2021
View reviewed changes
Copy link
Contributor

@ldziedziul ldziedziul left a comment

Choose a reason for hiding this comment

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

Nice work!

@hz-devops-test
Copy link

The job Hazelcast-pr-builder of your PR failed. (Hazelcast internal details: build log, artifacts).
Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log file 
--------------------------
-------TEST FAILURE-------
--------------------------
[INFO] Results:
[INFO] 
[ERROR] Failures: 
[ERROR]   QueueStatisticsTest.testAge:170 expected:<4611686018427387903> but was:<0>
[INFO] 
[ERROR] Tests run: 46513, Failures: 1, Errors: 0, Skipped: 1012
[INFO] 

[ERROR] There are test failures.

@hz-devops-test
Copy link

The job Hazelcast-pr-builder of your PR failed. (Hazelcast internal details: build log, artifacts).
Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log file 
--------------------------
-------TEST FAILURE-------
--------------------------
[INFO] Results:
[INFO] 
[ERROR] Failures: 
[ERROR]   QueueStatisticsTest.testAge:170 expected:<4611686018427387903> but was:<0>
[ERROR]   QueueStatisticsTest.testAge:170 expected:<4611686018427387903> but was:<0>
[INFO] 
[ERROR] Tests run: 46513, Failures: 2, Errors: 0, Skipped: 1012
[INFO] 

[ERROR] There are test failures.

frant-hartm
frant-hartm requested changes Nov 8, 2021
View reviewed changes
Copy link
Contributor

@frant-hartm frant-hartm left a comment

Choose a reason for hiding this comment

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

Thanks!

Could you extend the same logic to HAZELCAST_CLIENT_CONFIG in hz-cli file in the same directory?

@yuce
Copy link
Contributor Author

yuce commented Nov 8, 2021

@frant-hartm Updated at: 9a7e1af

@ldziedziul @frant-hartm Thanks for the review!

frant-hartm
frant-hartm approved these changes Nov 8, 2021
View reviewed changes
Copy link
Contributor

@frant-hartm frant-hartm left a comment

Choose a reason for hiding this comment

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

Thanks!

@yuce yuce merged commit 9b9c719 into hazelcast:master Nov 9, 2021
@yuce yuce deleted the hz-start-absolute-config branch November 9, 2021 06:20
frant-hartm pushed a commit to frant-hartm/hazelcast that referenced this pull request Dec 1, 2021
@yuce @frant-hartm
Allow absolute paths in HAZELCAST_CONFIG environment variable (hazelc…
1fcf7c9 
…ast#19908)

Allow absolute paths in HAZELCAST_CONFIG and HAZELCAST_CLIENT_CONFIG environment variables
@frant-hartm frant-hartm mentioned this pull request Dec 1, 2021
Merged
4 tasks
frant-hartm added a commit that referenced this pull request Dec 2, 2021
@frant-hartm @yuce
Use hazelcast.yaml when hazelcast.xml is not available [5.0.z] (#20004)
fc9864c 
* Allow absolute paths in HAZELCAST_CONFIG environment variable (#19908)

Allow absolute paths in HAZELCAST_CONFIG and HAZELCAST_CLIENT_CONFIG environment variables

* Use hazelcast.yaml when hazelcast.xml is not available

Users sometimes prefer to use yaml configuration.
When the hazelcast.xml is not available in the config folder then
hazelcast.yaml is tried.
If hazelcast.yaml is not available error message is shown with guidance
what to do.

Backport of #20003 and #19908 (for clean backport)

Fixes #19727

Co-authored-by: Yüce Tekol <yucetekol@gmail.com>
sancar pushed a commit to sancar/hazelcast that referenced this pull request Dec 13, 2021
@yuce
Allow absolute paths in HAZELCAST_CONFIG environment variable (hazelc…
e37f1a8 
…ast#19908)

Allow absolute paths in HAZELCAST_CONFIG and HAZELCAST_CLIENT_CONFIG environment variables
@mmedenjak mmedenjak added this to the 5.1 milestone Jan 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ldziedziul ldziedziul ldziedziul approved these changes

@frant-hartm frant-hartm frant-hartm approved these changes

Assignees
No one assigned
Labels
Estimation: S Source: Internal PR or issue was opened by an employee Team: Core Type: Enhancement
Projects
None yet
Milestone
5.1
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@yuce @hz-devops-test @ldziedziul @frant-hartm @mmedenjak