-
Notifications
You must be signed in to change notification settings - Fork 37
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
Added YAML configuration support and bumped Hazelcast to v4.0.2 #102
Conversation
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.
Added 2 minor comments. Other than that, LGTM 👍
@@ -32,7 +32,7 @@ | |||
<timestamp>${maven.build.timestamp}</timestamp> | |||
<extraVmArgs/> | |||
|
|||
<hazelcast.version>4.0</hazelcast.version> | |||
<hazelcast.version>4.0.2</hazelcast.version> |
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.
Can't we bump it to 4.1
?
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 planning to do it in another PR.
|
||
import static com.hazelcast.internal.config.ConfigLoader.locateConfig; | ||
|
||
class P2PConfigLoader { |
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.
Why is it called P2PConfigLoader
and not ConfigLoader
or MemberConfigLoader
?
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 know it doesn't look good, but member setup related classes are named as P2P
in tomcat-session-manager. Ex.: P2PLifecycleListener
.
We should reconsider this approach when this issue is fixed on IMDG side: hazelcast/hazelcast#17316 |
Fixes #93.