Closed
Bug 932346
Opened 11 years ago
Closed 11 years ago
Simple bug-change push extension
Categories
(bugzilla.mozilla.org Graveyard :: Bugzilla Change Notification System, defect)
bugzilla.mozilla.org Graveyard
Bugzilla Change Notification System
Production
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: mcote, Assigned: glob)
References
Details
Attachments
(1 file, 1 obsolete file)
8.10 KB,
patch
|
dkl
:
review+
|
Details | Diff | Splinter Review |
For the purposes of the Bugzilla Change Notification Service, we'd like a very very simple extension that pushes only the bug ID and delta_ts to a table. Another piece (filed as a separate bug) will read these entries and delete them from the table after processing.
Assignee: nobody → glob
Version: Development/Staging → Production
it has to be ZPushNotify to ensure it's run after all the other extensions have touched bugs in bug_end_of_update.
Attachment #824684 -
Flags: review?(dkl)
Comment 2•11 years ago
|
||
Comment on attachment 824684 [details] [diff] [review] 932346_1.patch Review of attachment 824684 [details] [diff] [review]: ----------------------------------------------------------------- Overall looks good and works in my testing. But I will r- as I feel it is also important when changing the dependencies on a bug that a row is inserted for the bug that is being blocked or depended on. Bugzilla::Bug::update will update the delta_ts of the dependent bugs when the relationship is created/removed but it doesn't use the normal update() method unfortunately. So something special will need to be done there. dkl ::: extensions/ZPushNotify/Extension.pm @@ +23,5 @@ > + $delta_ts //= $bug->delta_ts; > + Bugzilla->dbh->do( > + "INSERT INTO push_notify(bug_id, delta_ts) VALUES(?, ?)", > + undef, > + $bug->id, $delta_ts Suggestion: Would it be useful to first look if the bug_id already exists and update the current value if present? Will keep the table size down some. You could even do a unique index on bug_id,delta_ts and then use REPLACE INTO maybe. I know you mentioned that monitoring would be in place in case for whatever reason the table were to grow too large. If yes, new patch and I will test, otherwise not a show-stopper.
Attachment #824684 -
Flags: review?(dkl) → review-
this revision incorporates a change to Bugzilla::Bug to add a method for updating a bug's delta_ts, which we hook for this extension. if this looks sane i'll file an upstream bug for that part. it also adds a parameter (under advanced) to enable/disable this to avoid filling up push_notify tables in non-production environments. it's disabled by default. finally i followed your suggestion and use REPLACE INTO, with a corresponding schema change. the existing table to need to be manually deleted from the schema and the bz_schema entry.
Attachment #824684 -
Attachment is obsolete: true
Attachment #827882 -
Flags: review?(dkl)
Comment 4•11 years ago
|
||
Comment on attachment 827882 [details] [diff] [review] 932346_2.patch Review of attachment 827882 [details] [diff] [review]: ----------------------------------------------------------------- Looks real good and works as expected. r=dkl ::: Bugzilla/Bug.pm @@ +1148,5 @@ > + "UPDATE bugs SET delta_ts = ? WHERE bug_id = ?", > + undef, > + $timestamp, $bug_id > + ); > + Bugzilla::Hook::process('bug_delta_ts_updated', nit: For consistency maybe bug_delta_ts_end_of_update or is too long? ::: extensions/ZPushNotify/template/en/default/hook/admin/params/editparams-current_panel.html.tmpl @@ +7,5 @@ > + #%] > + > +[% IF panel.name == "advanced" %] > + [% panel.param_descs.enable_simple_push = > + 'When enabled bug changes will result in an entry in the <code>push_notify</code> table' containing the last updated timestamp.
Attachment #827882 -
Flags: review?(dkl) → review+
Reporter | ||
Comment 7•11 years ago
|
||
Will the push extension be writing the datetime in UTC or in the server's timezone? I ask because it appears that the bug history API returns dates in server's timezone but the history's new_since argument apparently expects UTC. Personally I would love it if this extension wrote in UTC, in part to ease the process of fetching the actual changes after getting the notification, but mostly because I think all dates not explicitly for presentation to the user should be in UTC.
(In reply to Mark Côté ( :mcote ) from comment #7) > Personally I would love it if this extension wrote in UTC, in part to ease > the process of fetching the actual changes after getting the notification, > but mostly because I think all dates not explicitly for presentation to the > user should be in UTC. i agree that all dates should be in UTC internally, however that isn't how bugzilla does things :( (bug 452353). it would cause confusion to have all dates in the database to be in one timezone with the exception of a single table, so this extension will follow bugzilla convention and store the delta_ts in the same timezone as the servers. i suggest asking mysql to convert the datetime to UTC when selecting it: SELECT bug_id, CONVERT_TZ(delta_ts,'US/Pacific','UTC') AS delta_ts, ...
Reporter | ||
Comment 9•11 years ago
|
||
Yeah true. Also I think I was confused when I said that the new_since takes a UTC argument; I believe the dev box is set to UTC, so I was probably testing there. I guess, actually, that we should keep the dates in their current format since presumably apps will be using that to query history. I absolutely hate the ambiguity around DST (e.g. the fact that 1:00 am on the first Sunday of November happens twice in Pacific time unless you specify PST vs PDT, which Bugzilla doesn't do) but that's bigger than this extension.
Assignee | ||
Comment 10•11 years ago
|
||
(In reply to Mark Côté ( :mcote ) from comment #9) > Yeah true. Also I think I was confused when I said that the new_since takes > a UTC argument; I believe the dev box is set to UTC, so I was probably > testing there. all dates returned by our webservices are UTC. for date parameters the timezone specified will be used, defaulting to the server's timezone if not provided. eg. ISO-8601 dates will be interpreted as UTC. Committing to: bzr+ssh://bjones%40mozilla.com@bzr.mozilla.org/bmo/4.2/ modified Bugzilla/Bug.pm added extensions/ZPushNotify added extensions/ZPushNotify/Config.pm added extensions/ZPushNotify/Extension.pm added extensions/ZPushNotify/template added extensions/ZPushNotify/template/en added extensions/ZPushNotify/template/en/default added extensions/ZPushNotify/template/en/default/hook added extensions/ZPushNotify/template/en/default/hook/admin added extensions/ZPushNotify/template/en/default/hook/admin/params added extensions/ZPushNotify/template/en/default/hook/admin/params/editparams-current_panel.html.tmpl Committed revision 9138.
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Updated•5 years ago
|
Product: bugzilla.mozilla.org → bugzilla.mozilla.org Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•