932346 - Simple bug-change push extension

Status: RESOLVED FIXED

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

Description

[Mark Côté [:mcote]]

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.

Comment 1

[:glob ✱]

Attachment: 932346_1.patch

it has to be ZPushNotify to ensure it's run after all the other extensions have touched bugs in bug_end_of_update.

Comment 2

[David Lawrence [: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.

Comment 3

[:glob ✱]

Attachment: 932346_2.patch

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.

Comment 4

[David Lawrence [: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.

Comment 7

[Mark Côté [:mcote]]

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.

Comment 8

[:glob ✱]

(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, ...

Comment 9

[Mark Côté [:mcote]]

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.

Comment 10

[:glob ✱]

(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://[e-mail]/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.