Shireesh Anjal has posted comments on this change.
Change subject: gluster: Use brick server for advanced details
......................................................................
Patch Set 4: (2 inline comments)
Sahina, yes - those UI related sources are required as they invoke the advanced
details query. That's why I added Gilad and Kanagaraj as reviewers, and Gilad
has already given +1
Omer, response to your comments inline. New patch-set to follow handling the
potential NPE pointed out by you.
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/gluster/GetGlusterVolumeAdvancedDetailsQuery.java
Line 38: @Override
Line 39: protected void executeQueryCommand() {
Line 40: clusterId = getParameters().getClusterId();
Line 41: detailRequired = getParameters().isDetailRequired();
Line 42: volumeId = getParameters().getVolumeId();
Sahina had suggested the same thing in patch-set 1, and here is my response:
"Sorry, can't really do this. Because the testing framework mocks both the
query and the parameters, moving these lines to the constructors starts failing
my tests.
There might be a way to make it work by moving these to constructor, but I
think there is no harm in keeping these here as well, as executeQueryCommand()
is the entry point for the functionality, and there is no chance that these
variables don't get populated. So will keep this code as is."
Let me know if you're aware of another query class (that has a proper test
case) which does the initialization in the constructor, and I'll see if I can
refer that and change my code accordingly.
Line 43: if (volumeId != null) {
Line 44: brick = getBrick(getParameters().getBrickId());
Line 45:
getQueryReturnValue().setReturnValue(fetchAdvancedDetails(getGlusterVolumeDao().getById(volumeId).getName()));
Line 46: } else {
Line 41: detailRequired = getParameters().isDetailRequired();
Line 42: volumeId = getParameters().getVolumeId();
Line 43: if (volumeId != null) {
Line 44: brick = getBrick(getParameters().getBrickId());
Line 45:
getQueryReturnValue().setReturnValue(fetchAdvancedDetails(getGlusterVolumeDao().getById(volumeId).getName()));
Done
Line 46: } else {
Line 47: getQueryReturnValue().setReturnValue(getServiceInfo());
Line 48: }
Line 49: }
--
To view, visit http://gerrit.ovirt.org/12979
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id4acb0bbe84bacab714926d69b639bbda970df9a
Gerrit-PatchSet: 4
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Shireesh Anjal <san...@redhat.com>
Gerrit-Reviewer: Gilad Chaplik <gchap...@redhat.com>
Gerrit-Reviewer: Kanagaraj M <kmayi...@redhat.com>
Gerrit-Reviewer: Michael Pasternak <mpast...@redhat.com>
Gerrit-Reviewer: Omer Frenkel <ofren...@redhat.com>
Gerrit-Reviewer: Sahina Bose <sab...@redhat.com>
Gerrit-Reviewer: Shireesh Anjal <san...@redhat.com>
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
