Report - Printing and Sending

Merge lp://qastaging/~numerigraphe-team/report-print-send/7.0-no-lock-in-update-1308635-ls into lp://qastaging/~report-print-send-core-editors/report-print-send/7.0

  1. 7.0-no-lock-in-update-1308635-ls
  2. Merge into 7.0
Proposed by Lionel Sausin - Initiatives/Numérigraphe
Status: Rejected
Rejected by: Pedro Manuel Baeza
Proposed branch: lp://qastaging/~numerigraphe-team/report-print-send/7.0-no-lock-in-update-1308635-ls
Merge into: lp://qastaging/~report-print-send-core-editors/report-print-send/7.0
Diff against target: 32 lines (+6/-11)
1 file modified
base_report_to_printer/printing.py (+6/-11)
To merge this branch: bzr merge lp://qastaging/~numerigraphe-team/report-print-send/7.0-no-lock-in-update-1308635-ls
Reviewer Review Type Date Requested Status
Yannick Vaucher @ Camptocamp moved on github Needs Resubmitting
Loïc Bellier - Numérigraphe (community) Abstain
Review via email: mp+216353@code.qastaging.launchpad.net

Description of the change

If I'm not mistaken, locking in printer.printer.update() is not needed, and the way it's written does not protect us from concurrent changes anyway :
- if what we read is immutable than the value is copied and locking is unnecessary
- if it's mutable then we're getting a "reference" to the shared object, and should keep the lock until we're completely done with it

From my quick code survey, the variables seem to contain a boolean and a float so I guess no locking is needed.

update() seems to be called pretty often (all the CRUD method call it) so It may help a bit to remove the contention.

To post a comment you must log in.
Revision history for this message
Loïc Bellier - Numérigraphe (lb-b) wrote :

Lionel,

Maybe you can have a conflict with line "for _ in range(0, 5):" on line 24
if the import "openerp.tools.translate import _" has been added ?

LB

review: Abstain
Revision history for this message
Yannick Vaucher @ Camptocamp (yvaucher-c2c) wrote :

LGTM appart for the variable _ though we are not using translation here.

review: Needs Fixing
Revision history for this message
Yannick Vaucher @ Camptocamp (yvaucher-c2c) wrote :

This project is now hosted on https://github.com/OCA/report-print-send. Please move your proposal there. This guide may help you https://github.com/OCA/maintainers-tools/wiki/How-to-move-a-Merge-Proposal-to-GitHub

review: Needs Resubmitting (moved on github)
Revision history for this message
Lionel Sausin - Initiatives/Numérigraphe (ls-initiatives) wrote :

Thanks!
For "_" I tried to express in a pythonic way that the variable is in
fact unused, but clearly this is not explicit enough.
I'll change to "_unused" and re-submit on github.

Revision history for this message
Lionel Sausin - Initiatives/Numérigraphe (ls-initiatives) wrote :

Githubified here:https://github.com/OCA/report-print-send/pull/9
Please reject this merge proposal on Launchpad to avoid confusion.

Unmerged revisions

13. By Numérigraphe

[IMP] remove useless locking from update(), shouldn't be needed for immutable data

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.