-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Introduce mode for persistent-memory configuration #17696
Conversation
@@ -433,7 +433,7 @@ | |||
min-block-size="10" | |||
page-size="20"> | |||
<hz:size unit="GIGABYTES" value="256"/> | |||
<hz:persistent-memory> | |||
<hz:persistent-memory enabled="false" mode="MOUNTED"> |
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.
If enabled flag is set to false
, why do we still have the directory
element underneath?
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.
We need to test parsing regardless of the value of enabled
. Probably it would be better to set it to true
instead since false
is the default.
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 kept false
instead, this way we have cases both for enabled and disabled persistent memory config.
throw new InvalidConfigurationException("Invalid 'mode' for 'persistent-memory': " + modeValue); | ||
} | ||
} | ||
|
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.
If enabledNode
is false, shall we go through the directories?
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 think yes. We need to build a config object hierarchy matching the declarative config.
@@ -28,11 +28,18 @@ | |||
* Configuration class for persistent memory devices (e.g. Intel Optane). | |||
*/ | |||
public class PersistentMemoryConfig { | |||
private boolean enabled; |
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.
Javadoc would be useful.
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
Introduce config attribute for the operational mode of the persistent memory. Two modes supported: `MOUNTED (FS DAX)` and `SYSTEM_MEMORY (KMEM DAX)`. With this setting, the user can define to use PMEM in the KMEM DAX mode. Additionally, an `enabled` toggle is added to `persistent-memory`, which makes it explicit when persisten memory is used. For legacy reasons, if the `persistent-memory-directory` that was introduced in 4.0 is set, `enabled` is set to `true`, so the users already using persistent memory don't need to change their configs. Otherwise, the persistent memory should be explicitly enabled by setting `enabled` to `true`, since the default is `false`.
9064d7a
to
1b877bc
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.
You covered all config aspects perfectly :)
hazelcast/src/main/java/com/hazelcast/internal/config/AbstractDomConfigProcessor.java
Outdated
Show resolved
Hide resolved
Thanks for the review @mmedenjak @petrpleshachkov @sancar 👍 |
Introduce config attribute for the operational mode of the persistent memory. Two modes supported:
MOUNTED (FS DAX)
andSYSTEM_MEMORY (KMEM DAX)
. With this setting, the user can define to use PMEM in the KMEM DAX mode. Additionally, anenabled
toggle is added topersistent-memory
, which makes it explicit when persisten memory is used. For legacy reasons, if thepersistent-memory-directory
that was introduced in 4.0 is set,enabled
is set totrue
, so the users already using persistent memory don't need to change their configs. Otherwise, the persistent memory should be explicitly enabled by settingenabled
totrue
, since the default isfalse
.EE PR: https://github.com/hazelcast/hazelcast-enterprise/pull/3814