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.

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

[WFCORE-2958] Add an addtional runtime step to remove formatters from… #2555

Closed
wants to merge 1 commit into from

Conversation

jamezp
Copy link
Member

@jamezp jamezp commented Jun 16, 2017

… a handler. In the case of a composite operation the formatter may be changed in a undefine-attribute operation followed by a removal of the formatter in a write-attribute handler for the named-formatter attribute. This causes the commit of the log managers configuration as the handler no longer exists after the write-attribute.

https://issues.jboss.org/browse/WFCORE-2958

@bstansberry
Copy link
Contributor

Storing per-op state in the OSH is pretty nasty. This may not work in the future as I don't think the kernel should have to support this kind of thing. If you want to stick with this I think a clear() call is needed.

@jamezp jamezp added the fixme This PR introduces issues that must be fixed label Jun 20, 2017
@jamezp
Copy link
Member Author

jamezp commented Jun 20, 2017

Good point this won't work right since the OSH is static. I'll have to think of another way to handle this. I just need the remove op to happen after the other ops have been committed in the config API. I don't think running in any other stage other than RUNTIME makes sense as it does make runtime changes.

@bstansberry
Copy link
Contributor

@jamezp Will an OperationContext attachment do the trick? Attach some sort of object that can store the post-commit steps, and whatever step is the one that doesn't find the attachment and adds it to the context is the one that registers a step to read the attachment and execute what it finds.

That attachment would be global to the overall operation, not targeted to a particular OSH (i.e. to one step in a composite), but I suspect that doesn't matter. And if it does the data in the attachment could be more complex to somehow track what resources added which steps.

@jamezp
Copy link
Member Author

jamezp commented Jun 20, 2017

@bstansberry Yeah that would work and makes sense. Thanks for the suggestion!

@jamezp jamezp removed the fixme This PR introduces issues that must be fixed label Jun 20, 2017
if (afterCommitSteps != null && !afterCommitSteps.isEmpty()) {
for (OperationStepHandler step : afterCommitSteps) {
context.addStep(step, Stage.RUNTIME);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this get called more than once in an overall op, such that that same after-commit steps get added multiple times? From the uses of addCommitStep it looks like that could happen.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. I meant to use detach here. Fix coming up.

for (OperationStepHandler step : afterCommitSteps) {
context.addStep(step, Stage.RUNTIME);
}
context.addStep(new CommitOperationStepHandler(configurationPersistence), Stage.RUNTIME);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be outside the if block?

Copy link
Member Author

Choose a reason for hiding this comment

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

No it should be inside the if block. I only need to add another commit step if there are other changes to commit.

… a handler. In the case of a composite operation the formatter may be changed in a undefine-attribute operation followed by a removal of the formatter in a write-attribute handler for the named-formatter attribute. This causes the commit of the log managers configuration as the handler no longer exists after the write-attribute.
@jamezp jamezp added the hold Do not merge this PR label Jun 21, 2017
@jamezp
Copy link
Member Author

jamezp commented Jun 21, 2017

I'm placing this back on hold because it could introduce issues with rollbacks if a composite op fails. If a step fails, the composite op could partially be committed to the logging configuration API leaving the config in a weird state.

@jamezp
Copy link
Member Author

jamezp commented Jun 21, 2017

I'm just going to close this one and open a new one. The is some other odd behavior with these two attributes that cause this issue. This should be properly fixed rather than hacked together.

@jamezp jamezp closed this Jun 21, 2017
@jamezp jamezp deleted the WFCORE-2958 branch June 23, 2017 23:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hold Do not merge this PR
Projects
None yet
2 participants