Nir Soffer has posted comments on this change. Change subject: misc: Safer and simpler itmap ......................................................................
Patch Set 3: (2 comments) https://gerrit.ovirt.org/#/c/39119/3//COMMIT_MSG Commit Message: Line 8: Line 9: The previous code had few issues: Line 10: Line 11: - It used unlimited number of threads by default. This may lead to Line 12: creation of 100's of threads if you do not specify a value. > s/100's/hundreds/, s/creation/the creation/ Done Line 13: - It used non-daemon threads, which could lead to unwanted delay during Line 14: vdsm shutdown. Line 15: - It tried to yield results before all arguments were handled. This Line 16: could lead to unwanted delay in argument processing, if the caller Line 19: number of values. Line 20: - If invalid maxthreads was given, the ValueError was intialized with a Line 21: tuple ("Wrong input to function itmap: %s", maxthreads) instead of Line 22: a formatted string. Line 23: - It was too complicated. > I'm not a fan of such vague statements. IMHO, either elaborate it or drop i I will make it more clear - It was too hard for me to understand how the code worked. This is the main motivation of this patch, get the code into a state I fill confident to use it. Line 24: Line 25: Changes: Line 26: Line 27: - The caller must specify the maximum number of threads. -- To view, visit https://gerrit.ovirt.org/39119 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iba6116ac4003702c8e921cebaf494491a6f9afaf Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Federico Simoncelli <fsimo...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: Yaniv Bronhaim <ybron...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches