Liron Aravot has posted comments on this change.
Change subject: Reinitialize DC doesn't connect all hosts to the new
master(#880180)
......................................................................
Patch Set 2: (2 inline comments)
mkublin, when user performs recovery he is performing reconstruct, those
commands means the same - copy the pool metadata to some domain and call it
master, recovery is just one corner case of reconstruct. right now there is no
need to copy the method in my opinion - 99% of it is common -
1. indeed the _LastMaster check in the if condition is redundant in case of
recovery, but the problem in my opinion is that we even get to this
connectAndRefreshHost method when we have no master - but this is not related
to this patch, this check can and should be removed from there in my opinion in
further patch that will seperate the connect/refresh operations from disconnect
operations.
2. exception handler - same for both.
3. isInactive() - same for both, when peforming recovery you will enter this if
clause (isInactive is set the false in recovery parameters) and you will enter
it also for reconstruct from ForceRemoveStorageDomain command.
basically copying this method to recovery will result in the same method which
we need to maintain in both classes, so i don't see a need to perform it now -
basically as recovery is reconstruct - not that i have problem copying it, just
don't think that it neccessary and will cause developers to need to remember to
fix each time
in both commands and have 99% same implementation
by the way regarding regression, i didn't see that ever recovery flow performed
connect storage server, so i think that in earlier version the hosts just
didn't went non operational , but still weren't connected to the new pool as
connect storage server wasn't performed.
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/RecoveryStoragePoolCommand.java
Line 83: }
Line 84:
Line 85: @Override
Line 86: protected boolean performConnectOperations(VDS vds) {
Line 87: if (vds.getId().equals(getVds().getId())
1. take a look of where this method is being used, it's being called for hosts
that as been loaded from the db - so they can't be null, this method shouldn't
be used outside of this flow and therefore vds can't be null - this check is
redundant.
2. the method does perform connect operations, take a look at the if clause :)
Line 88: ||
StorageHelperDirector.getInstance().getItem(getStorageDomain().getstorage_type())
Line 89:
.ConnectStorageToDomainByVdsId(getNewMaster(false), vds.getId())) {
Line 90: return true;
Line 91: }
Line 87: if (vds.getId().equals(getVds().getId())
Line 88: ||
StorageHelperDirector.getInstance().getItem(getStorageDomain().getstorage_type())
Line 89:
.ConnectStorageToDomainByVdsId(getNewMaster(false), vds.getId())) {
Line 90: return true;
Line 91: }
I've meant to write 'before' :-)
Line 92: log.errorFormat("Error while trying connect host {0} to the
needed storage server during the reinitialization of Data Center {1}",
Line 93: vds.getId(),
Line 94: getStoragePool().getId());
Line 95: return false;
--
To view, visit http://gerrit.ovirt.org/9748
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I3733b732b37f3a687b24f90bfdb1799757434d75
Gerrit-PatchSet: 2
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Liron Aravot <lara...@redhat.com>
Gerrit-Reviewer: Alissa Bonas <abo...@redhat.com>
Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com>
Gerrit-Reviewer: Ayal Baron <aba...@redhat.com>
Gerrit-Reviewer: Daniel Erez <de...@redhat.com>
Gerrit-Reviewer: Liron Aravot <lara...@redhat.com>
Gerrit-Reviewer: Maor Lipchuk <mlipc...@redhat.com>
Gerrit-Reviewer: Michael Kublin <mkub...@redhat.com>
Gerrit-Reviewer: Tal Nisan <tni...@redhat.com>
Gerrit-Reviewer: Vered Volansky <vvola...@redhat.com>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
