[webkit-reviews] review granted: [Bug 210425] Add slack-aware WKR implementation : [Attachment 396329] Patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has granted Yusuke Suzuki
<ysuzuki at apple.com>'s request for review:
Bug 210425: Add slack-aware WKR implementation
https://bugs.webkit.org/show_bug.cgi?id=210425

Attachment 396329: Patch

https://bugs.webkit.org/attachment.cgi?id=396329&action=review




--- Comment #19 from Alexey Proskuryakov <ap at webkit.org> ---
Comment on attachment 396329
  --> https://bugs.webkit.org/attachment.cgi?id=396329
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=396329&action=review

I'm going to say r+ because this is already running in production, and the
patch isn't introducing anything terrible like security problems. It does
appear that substantial changes or even a complete rewrite will be necessary
before moving to webkit.org infrastructure.

> Tools/WKR/WKR.mjs:36
> +const DEBUG = false;

You explained what this does, but my question was about how this is used. Would
a comment like this be accurate: "// Change to true before starting to avoid
posting notifications to Slack."?

> Tools/WKR/WKR.mjs:166
> +	   await sleep(Math.random() * 1000);

I don't understand how using random() here "avoids DDoS to Slack".

> Tools/WKR/WKR.mjs:191
> +	       // Exponential back-off capped with 1 hour.

You explained how "exponential back-off" behaves, but did not explain why a cap
of 1 hour is desirable. I still think that it is undesirable to delay coming
back up for up to an hour.

I think that this is addressing multiple scenarios:

1. git.webkit.org is not responding. It would be perfectly OK to keep making
the request every minute until it comes back up.

2. slack.com is not responding. It might be a problem if the whole world was
retrying every minute, to overwhelm Slack with all accumulated requests once
it's back up. Still, an hour is excessive.

3. Someone lands a syntax error in contributors.json, so it fails to parse. It
is desirable to keep checking frequently, no reason for exponential back-off.
Maybe WKR should handle this gracefully, by reusing the previous version of
contributors.json if trunk doesn't parse.