Alissa Bonas has posted comments on this change.
Change subject: core: fix redundant storage server conn in db
......................................................................
Patch Set 3: (5 inline comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/RemoveStorageServerConnectionCommand.java
Line 3: import org.apache.commons.lang.StringUtils;
Line 4: import org.ovirt.engine.core.bll.InternalCommandAttribute;
Line 5: import org.ovirt.engine.core.bll.NonTransactiveCommandAttribute;
Line 6: import
org.ovirt.engine.core.common.action.StorageServerConnectionParametersBase;
Line 7:
Because in some cases there is a need to just disconnect the vdsm from storage,
and in other cases (such as failure), you need to disconnect the storage AND
remove the connection from db for a clean up.
See bug mentioned in commit message that this patch fixes.
Line 8: @NonTransactiveCommandAttribute
Line 9: @InternalCommandAttribute
Line 10: public class RemoveStorageServerConnectionCommand<T extends
StorageServerConnectionParametersBase> extends
DisconnectStorageServerConnectionCommand {
Line 11:
Line 5: import org.ovirt.engine.core.bll.NonTransactiveCommandAttribute;
Line 6: import
org.ovirt.engine.core.common.action.StorageServerConnectionParametersBase;
Line 7:
Line 8: @NonTransactiveCommandAttribute
Line 9: @InternalCommandAttribute
It inherits from another command which is also internal, thus keeping the
convention.
Line 10: public class RemoveStorageServerConnectionCommand<T extends
StorageServerConnectionParametersBase> extends
DisconnectStorageServerConnectionCommand {
Line 11:
Line 12: public RemoveStorageServerConnectionCommand(T parameters) {
Line 13: super(parameters);
Line 14: }
Line 15:
Line 16: @Override
Line 17: protected void executeCommand() {
Line 18: //disconnect the connection from vdsm
Please use punctuation, so your comments can be understood properly.
The code that is reused here is inherited from another class, and that code
snippet returns a boolean, thus here it is also used in the same way.
Line 19: boolean isStorageDisconnectSucceeded = disconnectStorage();
Line 20:
Line 21: if(isStorageDisconnectSucceeded) {
Line 22: String connectionId = getConnection().getid();
Line 19: boolean isStorageDisconnectSucceeded = disconnectStorage();
Line 20:
Line 21: if(isStorageDisconnectSucceeded) {
Line 22: String connectionId = getConnection().getid();
Line 23: if(StringUtils.isNotEmpty(connectionId)) {
Disconnect on vdsm level succeeds regardless of the id because they disconnect
the path. Id is just a guid in db and you might receive a blank one.
Line 24: //remove the connection record from db
Line 25:
getDbFacade().getStorageServerConnectionDao().remove(connectionId);
Line 26: setSucceeded(true);
Line 27: }
....................................................
Commit Message
Line 11: The redundant entries entered when AddStorageServerConnectionCommand
Line 12: succeeded, but another command that ran after it failed. Adding a new
Line 13: command that in case of failure deletes the connection from db fixes
the
Line 14: problem.
Line 15: As part of the fix, add assignment of the connection id in UI in
StorageListModel -because otherwise it would not know which connection to
delete.
ok will fix, thanks
Line 16:
Line 17: Change-Id: Iea5468371514bd2c7bc043a6c5520e2864a09fe8
Line 18: Bug-Url: https://bugzilla.redhat.com/815083
--
To view, visit http://gerrit.ovirt.org/11936
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iea5468371514bd2c7bc043a6c5520e2864a09fe8
Gerrit-PatchSet: 3
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Alissa Bonas <[email protected]>
Gerrit-Reviewer: Alissa Bonas <[email protected]>
Gerrit-Reviewer: Maor Lipchuk <[email protected]>
Gerrit-Reviewer: Michael Kublin <[email protected]>
Gerrit-Reviewer: Yair Zaslavsky <[email protected]>
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
