Change in vdsm[master]: get max_tasks and thread_pool_size from config by getint()

ShaoHe Feng has uploaded a new change for review.

Change subject: get max_tasks and thread_pool_size from config by getint()
......................................................................

get max_tasks and thread_pool_size from config by getint()

It is OK to get max_tasks and thread_pool_size by getfloat()

The max_tasks will be passed to the Queue as maxsize and the
thread_pool_size is the thread number of the task thread pool.

So int is more reasonable than float

Change-Id: I9fc719b1b4238b0df2d9c882777cb75d7a27d4df
Signed-off-by: ShaoHe Feng 
---
M vdsm/storage/storage_mailbox.py
M vdsm/storage/taskManager.py
2 files changed, 6 insertions(+), 6 deletions(-)

git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/35/9035/1

diff --git a/vdsm/storage/storage_mailbox.py b/vdsm/storage/storage_mailbox.py
index c537883..1c8700d 100644
--- a/vdsm/storage/storage_mailbox.py
+++ b/vdsm/storage/storage_mailbox.py
@@ -208,9 +208,9 @@

def __init__(self, inbox, outbox, hostID, queue, monitorInterval):
         # Save arguments
-        tpSize = config.getfloat('irs', 'thread_pool_size') / 2
+        tpSize = config.getint('irs', 'thread_pool_size') / 2
         waitTimeout = 3
-        maxTasks = config.getfloat('irs', 'max_tasks')
+        maxTasks = config.getint('irs', 'max_tasks')
         self.tp = ThreadPool(tpSize, waitTimeout, maxTasks)
         self._stop = False
         self._flush = False
@@ -466,9 +466,9 @@
         self._stopped = False
         self._poolID = str(pool.spUUID)
         self._spmStorageDir = pool.storage_repository
-        tpSize = config.getfloat('irs', 'thread_pool_size') / 2
+        tpSize = config.getint('irs', 'thread_pool_size') / 2
         waitTimeout = 3
-        maxTasks = config.getfloat('irs', 'max_tasks')
+        maxTasks = config.getint('irs', 'max_tasks')
         self.tp = ThreadPool(tpSize, waitTimeout, maxTasks)
         #  *** IMPORTANT NOTE: The SPM's inbox is the HSMs' outbox and vice versa *** #
         self._inbox = os.path.join(self._spmStorageDir, self._poolID, "mastersd", sd.DOMAIN_META_DATA, "inbox")
diff --git a/vdsm/storage/taskManager.py b/vdsm/storage/taskManager.py
index 3bc12f3..f7a5da2 100644
--- a/vdsm/storage/taskManager.py
+++ b/vdsm/storage/taskManager.py
@@ -31,9 +31,9 @@
     log = logging.getLogger('TaskManager')

def __init__(self,
-                 tpSize=config.getfloat('irs', 'thread_pool_size'),
+                 tpSize=config.getint('irs', 'thread_pool_size'),
                  waitTimeout=3,
-                 maxTasks=config.getfloat('irs', 'max_tasks')):
+                 maxTasks=config.getint('irs', 'max_tasks')):
         self.storage_repository = config.get('irs', 'repository')
         self.tp = ThreadPool(tpSize, waitTimeout, maxTasks)
         self._tasks = {}

--
To view, visit http://gerrit.ovirt.org/9035
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I9fc719b1b4238b0df2d9c882777cb75d7a27d4df
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: ShaoHe Feng

Mark Wu has posted comments on this change.

Change subject: get max_tasks and thread_pool_size from config by getint()
......................................................................

Patch Set 1: Looks good to me, but someone else must approve

--
To view, visit http://gerrit.ovirt.org/9035
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I9fc719b1b4238b0df2d9c882777cb75d7a27d4df
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: ShaoHe Feng 
Gerrit-Reviewer: Mark Wu

Itamar Heim has posted comments on this change.

Change subject: get max_tasks and thread_pool_size from config by getint()
......................................................................

Patch Set 1:

still relevant or should be abandoned?

[Federico Simoncelli]

Federico Simoncelli has posted comments on this change.

Change subject: get max_tasks and thread_pool_size from config by getint()
......................................................................

Patch Set 1: Code-Review+1

OK, it seems that these values are already used to be compared with integers (=> round down) in __setThreadCountNolock and in Queue (maxsize).

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: get max_tasks and thread_pool_size from config by getint()
......................................................................

Patch Set 3: Verified-1

Build Failed

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/6729/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/6642/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/5837/ : FAILURE

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: get max_tasks and thread_pool_size from config by getint()
......................................................................

Patch Set 2:

Build Successful

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/6728/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/6641/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/5836/ : SUCCESS

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: get max_tasks and thread_pool_size from config by getint()
......................................................................

Patch Set 4:

Build Successful

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/6776/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_pep8_gerrit/6689/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/5884/ : SUCCESS

Adam Litke has posted comments on this change.

Change subject: get max_tasks and thread_pool_size from config by getint()
......................................................................

Patch Set 4: Verified+1

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: get max_tasks and thread_pool_size from config by getint()
......................................................................

Patch Set 4:

(2 comments)

http://gerrit.ovirt.org/#/c/9035/4//COMMIT_MSG
Commit Message:

Line 10: 
Line 11: The max_tasks will be passed to the Queue as maxsize and the
Line 12: thread_pool_size is the thread number of the task thread pool.
Line 13: 
Line 14: So int is more reasonable than float
That is clearly true.
Line 15: 
Line 16: Additionally, create a new configuration variable
Line 17: mailbox_thread_pool_size so the number of threads assigned to the
Line 18: storage mailbox can be set independently.

Line 14: So int is more reasonable than float
Line 15: 
Line 16: Additionally, create a new configuration variable
Line 17: mailbox_thread_pool_size so the number of threads assigned to the
Line 18: storage mailbox can be set independently.
Why do we need additional configurables? I know it's an ancient commit but it should have been splitted.
Line 19: 
Line 20: Change-Id: I9fc719b1b4238b0df2d9c882777cb75d7a27d4df
Line 21: Signed-off-by: ShaoHe Feng

Adam Litke has posted comments on this change.

Change subject: get max_tasks and thread_pool_size from config by getint()
......................................................................

Patch Set 4:

(1 comment)

http://gerrit.ovirt.org/#/c/9035/4//COMMIT_MSG
Commit Message:

Line 14: So int is more reasonable than float
Line 15: 
Line 16: Additionally, create a new configuration variable
Line 17: mailbox_thread_pool_size so the number of threads assigned to the
Line 18: storage mailbox can be set independently.
> Why do we need additional configurables? I know it's an ancient commit but

It was requested by Ayal.  Rather then make the storage_mailbox threadpool size tied to the current setting.
Line 19: 
Line 20: Change-Id: I9fc719b1b4238b0df2d9c882777cb75d7a27d4df
Line 21: Signed-off-by: ShaoHe Feng

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: get max_tasks and thread_pool_size from config by getint()
......................................................................

Patch Set 4: Code-Review-1

(1 comment)

http://gerrit.ovirt.org/#/c/9035/4//COMMIT_MSG
Commit Message:

Line 14: So int is more reasonable than float
Line 15: 
Line 16: Additionally, create a new configuration variable
Line 17: mailbox_thread_pool_size so the number of threads assigned to the
Line 18: storage mailbox can be set independently.
> It was requested by Ayal.  Rather then make the storage_mailbox threadpool

I am sure that Ayal had good reasons, and maybe an actual case where it is important to tweak this value. However, they are not clear to me. I'd rather have the constant "5" hard-coded than introduce a configurable that almost no one understands and (hopefully) no one uses.

Adam, would you please split it off this patch?

Nir, could you shed more light on this?
Line 19: 
Line 20: Change-Id: I9fc719b1b4238b0df2d9c882777cb75d7a27d4df
Line 21: Signed-off-by: ShaoHe Feng

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: get max_tasks and thread_pool_size from config by getint()
......................................................................

Patch Set 4:

(2 comments)

http://gerrit.ovirt.org/#/c/9035/4//COMMIT_MSG
Commit Message:

Line 10: 
Line 11: The max_tasks will be passed to the Queue as maxsize and the
Line 12: thread_pool_size is the thread number of the task thread pool.
Line 13: 
Line 14: So int is more reasonable than float
> That is clearly true.

What if you want exactly 3.2 threads?
Line 15: 
Line 16: Additionally, create a new configuration variable
Line 17: mailbox_thread_pool_size so the number of threads assigned to the
Line 18: storage mailbox can be set independently.

Line 14: So int is more reasonable than float
Line 15: 
Line 16: Additionally, create a new configuration variable
Line 17: mailbox_thread_pool_size so the number of threads assigned to the
Line 18: storage mailbox can be set independently.
> I am sure that Ayal had good reasons, and maybe an actual case where it is

I don't know anything about it but I don't see any reason to mix the thread pool size used for running tasks with the thread pool used for the mailbox.

I agree that we should split these patches and ask Ayal to explain why we need configurable mailbox thread pool.
Line 19: 
Line 20: Change-Id: I9fc719b1b4238b0df2d9c882777cb75d7a27d4df
Line 21: Signed-off-by: ShaoHe Feng

Adam Litke has posted comments on this change.

Change subject: get max_tasks and thread_pool_size from config by getint()
......................................................................

Patch Set 5: Verified+1

Split out the configuration change so that all that's left in this patch is the getfloat -> getint changes

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: get max_tasks and thread_pool_size from config by getint()
......................................................................

Patch Set 5: Verified-1

Build Failed

http://jenkins.ovirt.org/job/vdsm_storage_functional_tests_localfs/128/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/6249/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/7028/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/7139/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_storage_functional_tests_nfs/67/ : FAILURE

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: get max_tasks and thread_pool_size from config by getint()
......................................................................

Patch Set 5: Code-Review+2

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: get max_tasks and thread_pool_size from config by getint()
......................................................................

Patch Set 5: Code-Review+1

[Dan Kenigsberg]

Dan Kenigsberg has submitted this change and it was merged.

Change subject: get max_tasks and thread_pool_size from config by getint()
......................................................................

get max_tasks and thread_pool_size from config by getint()

It is OK to get max_tasks and thread_pool_size by getfloat()

The max_tasks will be passed to the Queue as maxsize and the
thread_pool_size is the thread number of the task thread pool.

So int is more reasonable than float

Change-Id: I9fc719b1b4238b0df2d9c882777cb75d7a27d4df
Signed-off-by: ShaoHe Feng 
Signed-off-by: Adam Litke 
Reviewed-on: http://gerrit.ovirt.org/9035
Reviewed-by: Dan Kenigsberg 
Reviewed-by: Nir Soffer 
---
M lib/vdsm/config.py.in
M vdsm/storage/storage_mailbox.py
M vdsm/storage/taskManager.py
3 files changed, 8 insertions(+), 7 deletions(-)

Approvals:
  Nir Soffer: Looks good to me, but someone else must approve
  Adam Litke: Verified
  Dan Kenigsberg: Looks good to me, approved