Closed Bug 932346 Opened 6 years ago Closed 6 years ago

Simple bug-change push extension

Categories

(bugzilla.mozilla.org Graveyard :: Bugzilla Change Notification System, defect)

Production
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: mcote, Assigned: glob)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Blocks: 879835
Assignee: nobody → glob
Version: Development/Staging → Production
Attached patch 932346_1.patch (obsolete) — Splinter Review
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 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-
Attached patch 932346_2.patchSplinter 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 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+
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, ...
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.
(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: 6 years ago
Resolution: --- → FIXED
Component: General → Bugzilla Change Notification System
Product: bugzilla.mozilla.org → bugzilla.mozilla.org Graveyard
You need to log in before you can comment on or make changes to this bug.