Change in vdsm[master]: gluster:refactor: User specified backup servers option

Ala Hino has uploaded a new change for review.

Change subject: gluster:refactor: User specified backup servers option
......................................................................

gluster:refactor: User specified backup servers option

Move handling user specified backup servers option from
_get_backup_servers_option method to options property.

Change-Id: I4238328cfe230852182a1d997bd51b00f686f4a8
Signed-off-by: Ala Hino 
---
M vdsm/storage/storageServer.py
1 file changed, 5 insertions(+), 5 deletions(-)

git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/03/43003/1

diff --git a/vdsm/storage/storageServer.py b/vdsm/storage/storageServer.py
index 3d62694..e71b88c 100644
--- a/vdsm/storage/storageServer.py
+++ b/vdsm/storage/storageServer.py
@@ -297,7 +297,11 @@

@property
     def options(self):
-        backup_servers_option = self._get_backup_servers_option()
+        if "backup-volfile-servers" in self._options:
+            self.log.warn("Using user specified backup-volfile-servers option")
+            backup_servers_option = ""
+        else:
+            backup_servers_option = self._get_backup_servers_option()
         return ",".join(
             p for p in (self._options, backup_servers_option) if p)

@@ -313,10 +317,6 @@
             raise se.UnsupportedGlusterVolumeReplicaCountError(replicaCount)

def _get_backup_servers_option(self):
-        if "backup-volfile-servers" in self._options:
-            self.log.warn("Using user specified backup-volfile-servers option")
-            return ""
-
         servers = [brick.split(":")[0] for brick in self.volinfo['bricks']]
         servers.remove(self._volfileserver)
         if not servers:

[e-mail] has posted comments on this change.

Change subject: gluster:refactor: User specified backup servers option
......................................................................

Patch Set 1:

* Update tracker::IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])

Ala Hino has posted comments on this change.

Change subject: gluster:refactor: User specified backup servers option
......................................................................

Patch Set 1: Verified+1

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: gluster:refactor: User specified backup servers option
......................................................................

Patch Set 1: Code-Review+1

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: gluster:refactor: User specified backup servers option
......................................................................

Patch Set 1:

(1 comment)

https://gerrit.ovirt.org/#/c/43003/1//COMMIT_MSG
Commit Message:

Line 6: 
Line 7: gluster:refactor: User specified backup servers option
Line 8: 
Line 9: Move handling user specified backup servers option from
Line 10: _get_backup_servers_option method to options property.
can you explain why the suggested code is better than the original one?
Line 11: 
Line 12: Change-Id: I4238328cfe230852182a1d997bd51b00f686f4a8

Ala Hino has posted comments on this change.

Change subject: gluster:refactor: User specified backup servers option
......................................................................

Patch Set 1:

(1 comment)

https://gerrit.ovirt.org/#/c/43003/1//COMMIT_MSG
Commit Message:

Line 6: 
Line 7: gluster:refactor: User specified backup servers option
Line 8: 
Line 9: Move handling user specified backup servers option from
Line 10: _get_backup_servers_option method to options property.
> can you explain why the suggested code is better than the original one?

To address Nir's comment:
"
This check should be in options() - you should not access self._options in many places, better access it only in options() and let other code access self.options.
"

Provided here:
https://gerrit.ovirt.org/#/c/41931/40/vdsm/storage/storageServer.py
Line 11: 
Line 12: Change-Id: I4238328cfe230852182a1d997bd51b00f686f4a8

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: gluster:refactor: User specified backup servers option
......................................................................

Patch Set 1:

(1 comment)

https://gerrit.ovirt.org/#/c/43003/1//COMMIT_MSG
Commit Message:

Line 6: 
Line 7: gluster:refactor: User specified backup servers option
Line 8: 
Line 9: Move handling user specified backup servers option from
Line 10: _get_backup_servers_option method to options property.
> To address Nir's comment:

Additionally, get_backup_servers_option() should do one thing. If we don't need to get this option, we should not call it. Having the logic in a higher level make the code easier to follow.
Line 11: 
Line 12: Change-Id: I4238328cfe230852182a1d997bd51b00f686f4a8

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: gluster:refactor: User specified backup servers option
......................................................................

Patch Set 1: Code-Review+2

[Dan Kenigsberg]

Dan Kenigsberg has submitted this change and it was merged.

Change subject: gluster:refactor: User specified backup servers option
......................................................................

gluster:refactor: User specified backup servers option

Move handling user specified backup servers option from
_get_backup_servers_option method to options property.

Change-Id: I4238328cfe230852182a1d997bd51b00f686f4a8
Signed-off-by: Ala Hino 
Reviewed-on: https://gerrit.ovirt.org/43003
Continuous-Integration: Jenkins CI
Reviewed-by: Nir Soffer 
Reviewed-by: Dan Kenigsberg 
---
M vdsm/storage/storageServer.py
1 file changed, 5 insertions(+), 5 deletions(-)

Approvals:
  Nir Soffer: Looks good to me, but someone else must approve
  Jenkins CI: Passed CI tests
  Dan Kenigsberg: Looks good to me, approved
  Ala Hino: Verified

[e-mail] has posted comments on this change.

Change subject: gluster:refactor: User specified backup servers option
......................................................................

Patch Set 2:

* Update tracker::IGNORE, no Bug-Url found
* Set MODIFIED::IGNORE, no Bug-Url found.