WFLY-1439 #4579
WFLY-1439 #4579
Conversation
|
Triggering build using a merge of 5d1de3d2cc9719cc8ee2d12492574cd86584ed0c on branch master: |
|
Build 7298 is now running using a merge of 5d1de3d2cc9719cc8ee2d12492574cd86584ed0c on branch master: |
| } | ||
| } | ||
| } | ||
|
|
||
| final ManagementClient client = managementClient.get(); | ||
| try { | ||
| for (ServerSetupTask instance : current) { | ||
| for (ServerSetupTask instance : setupTasksAll) { | ||
| setupTasksInForce.add(instance); |
There was a problem hiding this comment.
Is the "setupTasksInForce" supposed to contain the ServerSetupTask(s) whose setup was successfully invoked? If yes, then this:
setupTasksInForce.add(instance);
should happen after
instance.setup(....);
has been invoked on the next line.
There was a problem hiding this comment.
Well, nope, if code for some reason throws exception, we should give it a chance to do graceul cleanup.
There was a problem hiding this comment.
In that case, I don't understand why you need this separate collection at all. The code block here is effectively copying over all of "setupTasksAll" collection into the "setupTasksInForce" collection irrespective of whether or not there's an exception.
There was a problem hiding this comment.
Trick is that if "setup" performs N changes and only n-th throws exception for some reason, the n-1 changes need to be reverted, hence the "tearDown" must be called. If "setupTasksInForce.add" happen after "setup", "tearDown" could not be called, hence n-1 changes would still be there.
This could be handled by class level variable which would be used as "index" of last successful setup task, but its more transparent this way, in my opinion.
|
Is it just formatting changes to WebCERTTestsSecurityDomainSetup.java? It's better to not include the formatting changes to unaffected lines of code. |
|
Build 7298 outcome was SUCCESS using a merge of 5d1de3d2cc9719cc8ee2d12492574cd86584ed0c on branch master: |
|
Triggering build using a merge of a71c8ebff8f941b3bf9f62de7ff68bf50a921bfe on branch master: |
|
Build 7299 is now running using a merge of a71c8ebff8f941b3bf9f62de7ff68bf50a921bfe on branch master: |
|
Build 7299 outcome was FAILURE using a merge of a71c8ebff8f941b3bf9f62de7ff68bf50a921bfe on branch master: |
| setupTasksInForce.get(i).tearDown(client, container.getKey()); | ||
| } catch (Exception e) { | ||
| log.error("Setup task failed during tear down. Offending class '" + setupTasksAll.get(i) + "'", e); | ||
| e.printStackTrace(); |
There was a problem hiding this comment.
Well, CI jobs suggest something different, without this, some setup failures can spawn atleast 50 test failures..
|
retest this please |
|
Triggering build using a merge of 11a2b0a on branch master: |
|
Build 7311 is now running using a merge of 11a2b0a on branch master: |
|
Build 7311 outcome was SUCCESS using a merge of 11a2b0a on branch master: |
|
This looks reasonable |
|
Merged |
ServerSetupObserver leaks changes if exception is thrown
https://bugzilla.redhat.com/show_bug.cgi?id=967533