Join GitHub today
GitHub is home to over 40 million developers working together to host and review code, manage projects, and build software together.
Sign upWFCORE-2620 - Add ability to read computed runtime values of IO subsystem buffer-pool attributes #2697
Conversation
This comment has been minimized.
This comment has been minimized.
|
@ctomc Please take a look when you have a sec' ! :) |
This comment has been minimized.
This comment has been minimized.
|
Default values in the management API description are static. |
This comment has been minimized.
This comment has been minimized.
|
@bstansberry I took a little week to reflect on your comment, but I'm really not sure what you mean here. Would you mind elaborating ? :/ @ctomc btw, any thoughts from your side on this ? |
| @@ -85,36 +81,29 @@ | |||
| .setFlags(AttributeAccess.Flag.RESTART_ALL_SERVICES) | |||
| .setAllowExpression(true) | |||
| .setValidator(new IntRangeValidator(1, true, true)) | |||
| .setDefaultValue(new ModelNode(defaultBufferSize)) | |||
This comment has been minimized.
This comment has been minimized.
bstansberry
Oct 25, 2017
Contributor
This isn't ok (same with the others). Only static values are allowed.
This comment has been minimized.
This comment has been minimized.
|
@bstansberry thanks for the clarification - of course, now, it seems indeed very clear what was the issue :) - anyhow, I've fixed that, hopefully, in a good manner. Let me know if you see any other issue with this PR! |
| protected void performRuntime(OperationContext context, ModelNode operation, ModelNode model) | ||
| throws OperationFailedException { | ||
| final PathAddress address = PathAddress.pathAddress(operation.get(OP_ADDR)); | ||
| final String name = address.getLastElement().getValue(); |
This comment has been minimized.
This comment has been minimized.
jmesnil
Feb 12, 2018
Member
You can directly use org.jboss.as.controller.OperationContext#getCurrentAddressValue
| } | ||
|
|
||
| static final SimpleAttributeDefinition BUFFER_SIZE = new SimpleAttributeDefinitionBuilder(Constants.BUFFER_SIZE, ModelType.INT, true) | ||
| .setFlags(AttributeAccess.Flag.RESTART_ALL_SERVICES) | ||
| .setAllowExpression(true) | ||
| .setValidator(new IntRangeValidator(1, true, true)) | ||
| .setDefaultValue(DEFAULT_BUFFER_SIZE) |
This comment has been minimized.
This comment has been minimized.
jmesnil
Feb 12, 2018
Member
You can not set default values for these attributes as they are derived from Runtime.getRuntime().maxMemory().
Their actual values will depend on the VM they are running and will not be provide the same "default" across all servers in domain mode.
| .build(); | ||
| static final SimpleAttributeDefinition BUFFER_PER_SLICE = new SimpleAttributeDefinitionBuilder(Constants.BUFFER_PER_SLICE, ModelType.INT, true) | ||
| .setFlags(AttributeAccess.Flag.RESTART_ALL_SERVICES) | ||
| .setAllowExpression(true) | ||
| .setValidator(new IntRangeValidator(1, true, true)) | ||
| .setDefaultValue(DEFAULT_BUFFERS_PER_REGION) |
This comment has been minimized.
This comment has been minimized.
| .build(); | ||
| static final SimpleAttributeDefinition DIRECT_BUFFERS = new SimpleAttributeDefinitionBuilder(Constants.DIRECT_BUFFERS, ModelType.BOOLEAN, true) | ||
| .setFlags(AttributeAccess.Flag.RESTART_ALL_SERVICES) | ||
| .setAllowExpression(true) | ||
| .setDefaultValue(DEFAULT_DIRECT_BUFFERS) |
This comment has been minimized.
This comment has been minimized.
| private static final int defaultBufferSize; | ||
| private static final int defaultBuffersPerRegion; | ||
| private static final boolean defaultDirectBuffers; | ||
| static final int defaultBufferSize; |
This comment has been minimized.
This comment has been minimized.
jmesnil
Feb 12, 2018
Member
there is no need to store the primitive values as they are wrapped in the ModelNode fields
| .install(); | ||
|
|
||
| public void execute(OperationContext context, ModelNode operation) throws OperationFailedException { | ||
| if ( definition.getType() == ModelType.INT ) |
This comment has been minimized.
This comment has been minimized.
jmesnil
Feb 12, 2018
Member
you need here to find the name of the attribute to read and return one of DEFAULT_BUFFERS_PER_REGION, DEFAULT_BUFFER_SIZE or DEFAULT_DIRECT_BUFFERS
This comment has been minimized.
This comment has been minimized.
|
@jmesnil thanks for the feedback! I've rework my implementation according to it (and also found a but at the same time). Let me know if you think I can still improve on this |
This comment has been minimized.
This comment has been minimized.
|
retest this please |
1 similar comment
This comment has been minimized.
This comment has been minimized.
|
retest this please |
| final boolean direct = directModel.isDefined() ? directModel.asBoolean() | ||
| : BufferPoolResourceDefinition.DEFAULT_DIRECT_BUFFERS.asBoolean(); | ||
|
|
||
| System.out.println("BufferSize:" + bufferSize + ", bufferPerSlice:" + bufferPerSlice + ", direct:" + direct); |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
|
||
| System.out.println("BufferSize:" + bufferSize + ", bufferPerSlice:" + bufferPerSlice + ", direct:" + direct); | ||
| final BufferPoolService service = new BufferPoolService(bufferSize, bufferPerSlice, direct); | ||
| System.out.println("RPE>>>> name:" + name); |
This comment has been minimized.
This comment has been minimized.
|
|
||
|
|
||
| /*<buffer-pool name="default" buffer-size="1024" buffers-per-slice="1024"/>*/ | ||
| static final SimpleAttributeDefinition BUFFER_SIZE = new SimpleAttributeDefinitionBuilder(Constants.BUFFER_SIZE, |
This comment has been minimized.
This comment has been minimized.
jmesnil
Feb 27, 2018
Member
putting everything on the same line makes it hard to read and mantain.
Could you revert to the previous formatting, please?
This comment has been minimized.
This comment has been minimized.
rpelisse
Feb 27, 2018
Author
Contributor
I think the Eclipse formatter screwed me on this one. I'll fix it indeed, thanks!
|
|
||
| static List<SimpleAttributeDefinition> ATTRIBUTES = Arrays.asList(BUFFER_SIZE, BUFFER_PER_SLICE, DIRECT_BUFFERS); |
This comment has been minimized.
This comment has been minimized.
| new BufferPoolAdd(), | ||
| new ReloadRequiredRemoveStepHandler() | ||
| ); | ||
| super(IOExtension.BUFFER_POOL_PATH, IOExtension.getResolver(Constants.BUFFER_POOL), BufferPoolAdd.INSTANCE, |
This comment has been minimized.
This comment has been minimized.
| private BufferPoolAdd() { | ||
| super(BufferPoolResourceDefinition.ATTRIBUTES); | ||
| private ModelNode returnConfigIfDefinedOrDefaultValueFor(String attributeName, ModelNode config, ModelNode defaultValue) { | ||
| System.out.println("RPE>>> config item is: " + config); |
This comment has been minimized.
This comment has been minimized.
| private static class BufferPoolAdd extends AbstractAddStepHandler { | ||
| @Override | ||
| public void registerAttributes(ManagementResourceRegistration resourceRegistration) { | ||
| resourceRegistration.registerReadOnlyAttribute(BUFFER_SIZE, new BufferReadAttributeHandler(BUFFER_SIZE)); |
This comment has been minimized.
This comment has been minimized.
jmesnil
Feb 27, 2018
Member
are they read-only? They are defined in the model and the user can define values when the buffer-pool resource is added.
This comment has been minimized.
This comment has been minimized.
rpelisse
Feb 27, 2018
Author
Contributor
Yes, the attributes should be read-only, @ctomc mentioned that on the upstream issue (if I understood correctly, of course).
This comment has been minimized.
This comment has been minimized.
jmesnil
Feb 27, 2018
Member
That's not what he said :)
I'm paraphrasing him as he said that there is no need to define a custom write operation handler as the buffer pool can not be dynamically resized.
However the attribute must still use a default write operation that will modify the model and put the server in reload-required (so that the new value will be only taken into account when the server is reloaded).
This comment has been minimized.
This comment has been minimized.
rpelisse
Feb 27, 2018
Author
Contributor
Of course! I've jumped from "can't be change dynamically" to "readonly" but it's not the same. My bad. I'll change that too...
| context.getCapabilityServiceTarget().addCapability(IO_POOL_RUNTIME_CAPABILITY, service) | ||
| .setInitialMode(ServiceController.Mode.ACTIVE) | ||
| .install(); | ||
| final ModelNode config = Resource.Tools.readModel(context.readResourceFromRoot(address)); |
This comment has been minimized.
This comment has been minimized.
jmesnil
Feb 27, 2018
Member
code seems a bit convoluted.
Why don't you just call the definition.resolveModelAttribute(context, model)?
If it is defined, returns the value, else, returns one of the DEFAULT_XXX that you could pass to the class when you instantiate it with the corresponding attribute definition?
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
@jmesnil OK, I think I've fixed everything. I've cleaned up the code of the Reader, I've added a Writer and made any change require a reload. Let me see if you see anything more I can enhance! |
This comment has been minimized.
This comment has been minimized.
wildfly-ci
commented
Mar 27, 2018
|
Core - Full Integration Build 7021 outcome was FAILURE using a merge of 704bea9 |
This comment has been minimized.
This comment has been minimized.
|
retest this please |
This comment has been minimized.
This comment has been minimized.
wildfly-ci
commented
Mar 27, 2018
|
Core - Full Integration Build 7022 outcome was FAILURE using a merge of 9607057 Failed tests
|
This comment has been minimized.
This comment has been minimized.
|
The tests failure seems unrelated. In the meantime... @jmesnil @bstansberry do you still have some changes request? I think I've covered all you mentioned but maybe I've missed something (or introduce an other problem :/ ) |
This comment has been minimized.
This comment has been minimized.
wildfly-ci
commented
Oct 19, 2018
|
Core - Full Integration Build 8077 outcome was FAILURE using a merge of d198161 Failed tests
|
This comment has been minimized.
This comment has been minimized.
|
Just for clarity purpose: I've rebased this change, but I still have to work on it (and won't before at least a couple of weeks). So please disregard this PR for now. |
…stem buffer-pool attributes
This comment has been minimized.
This comment has been minimized.
|
Hello @rpelisse I am just looking to clean up some of the PRs in the queue, is there likely to be any progress on this one in the short term? If not would it better to close this one and either reopen or resubmit once ready? |
| @@ -0,0 +1,83 @@ | |||
| package org.wildfly.extension.io; | |||
This comment has been minimized.
This comment has been minimized.
darranl
Apr 8, 2019
Contributor
When this is ready to be looked at this file is missing it's copyright header.
This comment has been minimized.
This comment has been minimized.
|
Yes, let's close this! This is a bit old and I have no time right now to explore it again. If somehow it becomes important, let me know, otherwise I'll set up a new PR when I can. |
rpelisse commentedAug 8, 2017
Issue: https://issues.jboss.org/browse/JBEAP-6984
Upstream Issue: https://issues.jboss.org/browse/WFCORE-2620
No upstream PR required
This changes is (hopefully) a better proposal than my previous changes. It is however just a draft - I don't expect it to be perfect, I'm sharing it to get feedback on what is wrong (and hopefully how to fix it). So please don't merge without thorough code review, thanks !