ARTEMIS-5100 support journal-max-io on 'create' CLI command #5455
Conversation
| @@ -327,6 +327,9 @@ public String[] getStaticNodes() { | |||
| @Option(names = "--jdbc-lock-expiration", description = "Lock expiration (in milliseconds).") | |||
| long jdbcLockExpiration = ActiveMQDefaultConfiguration.getDefaultJdbcLockExpirationMillis(); | |||
|
|
|||
| @Option(names = "--aio-journal-max-io", description = "The journal-max-io value to use when also using the ASYNCIO journal-type. When using NIO or MAPPED this value is always '1'. Default: 4096") | |||
There was a problem hiding this comment.
what about --journal-max-aio?
(journal max asynchronous io).
jbertram
force-pushed
the
ARTEMIS-5100
branch
2 times, most recently
from
da89995 to
3573581
Compare
...cli/src/test/java/org/apache/activemq/artemis/cli/commands/CreateTestIndividualSettings.java
Outdated
Show resolved
Hide resolved
...cli/src/test/java/org/apache/activemq/artemis/cli/commands/CreateTestIndividualSettings.java
Outdated
Show resolved
Hide resolved
| @@ -327,6 +327,9 @@ public String[] getStaticNodes() { | |||
| @Option(names = "--jdbc-lock-expiration", description = "Lock expiration (in milliseconds).") | |||
| long jdbcLockExpiration = ActiveMQDefaultConfiguration.getDefaultJdbcLockExpirationMillis(); | |||
|
|
|||
| @Option(names = "--journal-max-aio", description = "The journal-max-io value to use when also using the ASYNCIO journal-type. When using NIO or MAPPED this value is always '1'. Default: 4096") | |||
There was a problem hiding this comment.
Regardless what it is called, I would move this up beside the other journal related options rather than adding it after all the jdbc options (if there isnt some constraint preventing it).
The configuration impl class uses a suffix for its fields based on the journal type, JournalMaxIO_NIO and JournalMaxIO_AIO, so I'd wonder if such suffix naming convention transfers over to broker-properties usage somehow and if so might --journal-max-io-aio fit with it for alignment?
There was a problem hiding this comment.
Using the same pattern as broker properties would yield something like --journal-max-io_aio or maybe --journal-max-io-aio. I think the latter would be OK. It's one of the options I originally considered.
There was a problem hiding this comment.
Yeah, I think journal-max-io-aio would be fine. I think I'd at least try to make those two use cases align if possible, if it it is not just going to be using --journal-max-io to match the xml (which I also think would be fine, I dont think its any more confusing than the XML element always being named and being present even if its not doing anything; the CLI description makes clear enough when the option applies, and it wont have any effect in non-applicable cases regardless what it is named).
There was a problem hiding this comment.
I prefer journal-max-aio
io-aio sounds redundant to me.
There was a problem hiding this comment.
io-aio sounds redundant to me.
It is a little redundant, but it's no more redundant than Configuration.setJournalMaxIO_AIO, Configuration.setJournalMaxIO_NIO, etc. which is also reflected in broker properties.
There was a problem hiding this comment.
It is a little redundant, but also more consistent and less likely to confuse.
For me --journal-max-aio is a bit off because it is so similar to, but still not the same as, the actual journal-max-io XML element setting it is actually for controlling. I expect that would trip some folks up, both in finding it, and in using it. I would just use --journal-max-io in preference to --journal-max-aio personally. The different ways of configuring journal-max-io would then at least all include journal-max-io in their naming in some form, either partially or as the whole name.
If we use journal-max-aio we will then have 3 different names for configuration for the same thing, one of which doesnt even contain the name journal-max-io despite directly existing to govern that XML element.
There was a problem hiding this comment.
I don't think there's a direct reference between the Create command and their XML elements... these are distinct namespaces to me.
--journal-max-io-aio won't make it much better... if we are concerned we would have to find a totally different name such as --journal-limit-aio...
i think --journal-max-aio for this case is fine.. since there are no other similar parameters on the Create
that's my opinion on this, but I'm not strong opinionated ... if you guys want to go against what I think... I wouldn't fight over :)
There was a problem hiding this comment.
I settled on --journal-max-io just like we already have in the XML. Although it's not perfectly descriptive at least it's consistent, and the help text should provide enough information to clarify how to use it.
jbertram
force-pushed
the
ARTEMIS-5100
branch
3 times, most recently
from
e1cb57c to
bd70e7d
Compare
artemis-cli/src/main/java/org/apache/activemq/artemis/cli/commands/Create.java
Outdated
Show resolved
Hide resolved
jbertram
force-pushed
the
ARTEMIS-5100
branch
from
bd70e7d to
70d1d34
Compare
I struggled to decide the name of the input, ultimately settling on
--aio-journal-max-ioto clarify that it only applies tojournal-max-iowhen using aio.