Bug 186823 – Implement EWS real-time patch status updates and tail logging

• Component: Tools / Tests
• Status: RESOLVED WONTFIX

[Daniel Bates]

We should support dynamic updating of patch status so that a person can see the state of a patch as it is being processed by an EWS bot without needing to refresh the page. Moreover we should support tail logging of standard output and standard error streams when the EWS bots runs a command (say, to build a patch).

[Daniel Bates]

Created attachment 343111 [details]
Patch and unit tests

[EWS Watchlist]

Attachment 343111 [details] did not pass style-queue:


ERROR: Tools/QueueStatusServer/handlers/updatestatus.py:70:  [UpdateStatus.post] Instance of 'UpdateStatus' has no 'request' member  [pylint/E1101] [5]
ERROR: Tools/QueueStatusServer/handlers/updatestatus.py:72:  [UpdateStatus.post] Instance of 'UpdateStatus' has no 'response' member  [pylint/E1101] [5]
Total errors found: 2 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Daniel Bates]

<rdar://problem/29490561>

[EWS Watchlist]

Comment on attachment 343111 [details]
Patch and unit tests

Attachment 343111 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8274329

New failing tests:
http/tests/security/canvas-remote-read-remote-video-blocked-no-crossorigin.html

[EWS Watchlist]

Created attachment 343221 [details]
Archive of layout-test-results from ews200 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews200  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Dean Johnson]

Comment on attachment 343111 [details]
Patch and unit tests

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

Overall, the patch looks really good and this functionality will be great to have for EWS. I have some design concerns around storage of / access to logs after they expire from memcached, but those can be addressed in a future patch as this is a large improvement to the existing system.

> Tools/ChangeLog:41
> +        streams to the database if a patch fails to build of causes tests failures.

Maybe once the output is finished, we can compress the data and offer it as a gzip stream instead? It seems it would be useful to have full logs if possible. Then those archives could be cleaned up once they are >30 days old, or something similar.

> Tools/QueueStatusServer/model/taillog.py:43
> +        # Three hours should be a reasonable amount of time to build and test a WebKit change.

Please correct me if I'm wrong here, but this TailLog class will be used to track a single file, such as a build or test log, right? If so, small nit on 'build and test' -> 'build or test', since this is tracking a single file from a build or test run.

> Tools/QueueStatusServer/model/taillog.py:59
> +        if not memcache.get(key):

Minor nit: This could be simplified by inverting the logic like so:
if memcache.get(key):
    memchache.delete(key)

> Tools/QueueStatusServer/model/taillog.py:64
> +        if not memcache.get(self._key):

How are the return values 'True' and 'False' supposed to be used here? I don't see anywhere you use the return value of this function (at least in this patch).

Can you also clarify what benefits 'is_transient' offers? As the memcached entry only lasts 3 hours, it feel like any message can already be defined as transient since it will expire in <= 3 hours.

Edit: After reading the commit message I understand a little more of what 'is_transient' means here. I still recommend trying to find a way to improve the wording (or adding a comment) that emphasizes most content is thrown away by default, except for warnings/errors (if I understood correctly).

> Tools/QueueStatusServer/templates/patch.html:70
> +  }

Is this always true, or only when the memcache entry has expired? Also, is log information not preserved by memcached upon reload? I would hope someone could access full build logs from a recent run (and ideally from >=3 hours as well).

> Tools/Scripts/webkitpy/common/system/executive.py:129
>          if quiet:

For your consideration, I think this logic may read simpler if designed like this:
tee_stdout = open(os.devnull, 'w') if quiet else sys.stdout

This does have the downside of not being as explicit in the finally block below when calling `if quiet: tee_stdout.close()`, so I'm not in love with the change and leave it up to you as something to consider.

> Tools/Scripts/webkitpy/common/system/executive.py:133
> +            command_output = self.run_with_teed_output_and_throw_if_fail(args, tee_stdout, decode_output=decode_output, **kwargs)

Warning: This isn't recoverable if `self.run_with_teed_output_and_throw_if_fail` raises an exception as there is no except block to allow errors. Is there a case where that may be desirable here?

> Tools/Scripts/webkitpy/common/system/executive.py:139
> +    def run_with_teed_output_and_throw_if_fail(self, args, tee_stdout, decode_output, **kwargs):

Nit: throw -> raise. You may also consider replacing `if` with `on`, but I don't feel strongly about it.

> Tools/Scripts/webkitpy/common/system/executive_mock.py:102
> +    def run_with_teed_output_and_throw_if_fail(self, args, tee_stdout, quiet=False, cwd=None, env=None):

Did you mean `tee` rather than `teed` in the function name?

> Tools/Scripts/webkitpy/common/system/executive_mock.py:104
> +            env_string = ""

Nit: For your consideration you could simplify this statement as:
`env_string = "" if not env else ", env=%s" % env`

It may also read better using format, but I'm not familiar with the usual paradigms used here so won't recommend one of the other:
`env_string = " if not env else ", env={}".format(env)

I also recommend checking for `if env is None` in the compressed if block instead of `if not env` based on previous discussion we've had, but leave it up to you on which paradigm you prefer.

> Tools/Scripts/webkitpy/tool/bot/commitqueuetask_unittest.py:52
> +    def run_command(self, command, patch):

It's not immediately obvious to me what 'patch' is in the context of running a command. Ditto to all other occurrences of `patch` in `run_command`.

> Tools/Scripts/webkitpy/tool/commands/queues_unittest.py:89
> +        teed_stdout = WritableStatusServerStatusFileObject(tool.status_server, queue.name, patch)

Did you mean to use 'teed' instead of 'tee' here?

[Daniel Bates]

(In reply to Dean Johnson from comment #6)
> Comment on attachment 343111 [details]
> Patch and unit tests
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=343111&action=review
> 
> Overall, the patch looks really good and this functionality will be great to
> have for EWS. I have some design concerns around storage of / access to logs
> after they expire from memcached, but those can be addressed in a future
> patch as this is a large improvement to the existing system.
> 
> > Tools/ChangeLog:41
> > +        streams to the database if a patch fails to build of causes tests failures.
> 
> Maybe once the output is finished, we can compress the data and offer it as
> a gzip stream instead? It seems it would be useful to have full logs if
> possible. Then those archives could be cleaned up once they are >30 days
> old, or something similar.
> 

Notice that the EWS bot will upload the last 5000 combined characters of standard output and standard error for a build or test failure to the status server and we persist this data to disk. One example of this can be seen at <https://webkit-queues.webkit.org/results/8341523>. We could look to store more of output, including the full output, with or without compression. Regardless, I suggest we do such an enhancement in a follow up bug.

> > Tools/QueueStatusServer/model/taillog.py:43
> > +        # Three hours should be a reasonable amount of time to build and test a WebKit change.
> 
> Please correct me if I'm wrong here, but this TailLog class will be used to
> track a single file, such as a build or test log, right? 

No, we create one TailLog object for entire duration that patch is processed by a queue. The underlying channel (crested channel.create_channel()) lives for 3 hours (maybe we should increase this limit? maybe use the max limit?). The token returned by channel.create_channel() is stored in memcache and lives until the queue releases the patch or is evicted due to memory pressure or a cache flush, whichever occurs earlier.

> If so, small nit on
> 'build and test' -> 'build or test', since this is tracking a single file
> from a build or test run.
> 

Will keep the current wording. See above remark for reason.

> > Tools/QueueStatusServer/model/taillog.py:59
> > +        if not memcache.get(key):
> 
> Minor nit: This could be simplified by inverting the logic like so:
> if memcache.get(key):
>     memchache.delete(key)
> 

Actually, it seems sufficient to call memcache.delete() unconditionally since memcache makes no lifetime guarantees on the data stored in it. Will change the code in this function to read:

[[
@classmethod
def expire(cls, queue_name, attachment_id):
	# Deletion is a best effort operation. Memcache may be unavailable or the token may have
	# been evicted due to memory pressure or an explicit flushing of the cache.
	memcache.delete(cls._generate_key(queue_name, attachment_id))
]]

> > Tools/QueueStatusServer/model/taillog.py:64
> > +        if not memcache.get(self._key):
> 
> How are the return values 'True' and 'False' supposed to be used here? I
> don't see anywhere you use the return value of this function (at least in
> this patch).
> 

The purpose of this code is to check for key existence. As it turns our memcache does not have a has(key) API to do this. Towards making this clearer I will add the following member function:

    def _is_valid(self):
        return memcache.get(self._key) is not None

And will update the code in this TailLog.write_message() to read:

    if not self._is_valid():
        return False

> Can you also clarify what benefits 'is_transient' offers? As the memcached
> entry only lasts 3 hours, it feel like any message can already be defined as
> transient since it will expire in <= 3 hours.
> 
> Edit: After reading the commit message I understand a little more of what
> 'is_transient' means here. I still recommend trying to find a way to improve
> the wording (or adding a comment) that emphasizes most content is thrown
> away by default, except for warnings/errors (if I understood correctly).
> 

Where do you propose I add such a comment? 

> > Tools/QueueStatusServer/templates/patch.html:70
> > +  }
> 
> Is this always true, or only when the memcache entry has expired? 

Updates are pushed to the channel as they happen and are not stored in memory or on disk. So, transient message (used for standard output/standard error output) will be lost. Note that we will display all non-transient messages on page reload.

> log information not preserved by memcached upon reload? I would hope someone
> could access full build logs from a recent run (and ideally from >=3 hours
> as well).
> 
> > Tools/Scripts/webkitpy/common/system/executive.py:129
> >          if quiet:
> 
> For your consideration, I think this logic may read simpler if designed like
> this:
> tee_stdout = open(os.devnull, 'w') if quiet else sys.stdout

I prefer to keep the as-is because of the contextual information afforded by the placement of the FIXME comment.

> 
> This does have the downside of not being as explicit in the finally block
> below when calling `if quiet: tee_stdout.close()`, so I'm not in love with
> the change and leave it up to you as something to consider.
> 
> > Tools/Scripts/webkitpy/common/system/executive.py:133
> > +            command_output = self.run_with_teed_output_and_throw_if_fail(args, tee_stdout, decode_output=decode_output, **kwargs)
> 
> Warning: This isn't recoverable if
> `self.run_with_teed_output_and_throw_if_fail` raises an exception as there
> is no except block to allow errors. Is there a case where that may be
> desirable here?
> 

This code was explicitly written so that we let the exception propagate to the caller to handle. We only need to take care to do some cleanup hence the existence of the finally clause.

> > Tools/Scripts/webkitpy/common/system/executive.py:139
> > +    def run_with_teed_output_and_throw_if_fail(self, args, tee_stdout, decode_output, **kwargs):
> 
> Nit: throw -> raise. You may also consider replacing `if` with `on`, but I
> don't feel strongly about it.
> 

Will keep as is to match the naming of the exiting function Executive._run_command_with_teed_output().

> > Tools/Scripts/webkitpy/common/system/executive_mock.py:102
> > +    def run_with_teed_output_and_throw_if_fail(self, args, tee_stdout, quiet=False, cwd=None, env=None):
> 
> Did you mean `tee` rather than `teed` in the function name?
> 

Will rename tee_stdout to teed_stdout.

> > Tools/Scripts/webkitpy/common/system/executive_mock.py:104
> > +            env_string = ""
> 
> Nit: For your consideration you could simplify this statement as:
> `env_string = "" if not env else ", env=%s" % env`
> 
> It may also read better using format, but I'm not familiar with the usual
> paradigms used here so won't recommend one of the other:
> `env_string = " if not env else ", env={}".format(env)
> 

I'll consider it.

> I also recommend checking for `if env is None` in the compressed if block
> instead of `if not env` based on previous discussion we've had, but leave it
> up to you on which paradigm you prefer.
> 

Will keep as is. Using `if env is None` will not produce the same result when env := {} and I would need to check correctness and/or rebase the expected results of at least 16 tests:

webkitpy.tool.bot.irc_command_unittest.IRCCommandTest.test_rollout_updates_working_copy
webkitpy.tool.commands.download_unittest.DownloadCommandsTest.test_land_cowhand
webkitpy.tool.commands.download_unittest.DownloadCommandsTest.test_apply_watch_list
webkitpy.tool.commands.download_unittest.DownloadCommandsTest.test_check_style
webkitpy.tool.steps.steps_unittest.StepsTest.test_build_jsc
webkitpy.tool.steps.steps_unittest.StepsTest.test_runtests_api_debug
webkitpy.tool.steps.steps_unittest.StepsTest.test_runtests_debug_args
webkitpy.tool.steps.steps_unittest.StepsTest.test_runtests_api
webkitpy.tool.steps.steps_unittest.StepsTest.test_runtests_webkitpy
webkitpy.tool.steps.steps_unittest.StepsTest.test_runtests_bindings
webkitpy.tool.steps.steps_unittest.StepsTest.test_runtests_jsc_debug
webkitpy.tool.steps.commit_unittest.CommitTest.test_check_test_expectations
webkitpy.tool.steps.runtests_unittest.RunTestsTest.test_webkit_run_unit_tests
webkitpy.tool.steps.steps_unittest.StepsTest.test_build_jsc_debug
webkitpy.tool.steps.steps_unittest.StepsTest.test_runtests_args
webkitpy.tool.steps.steps_unittest.StepsTest.test_runtests_jsc

> > Tools/Scripts/webkitpy/tool/bot/commitqueuetask_unittest.py:52
> > +    def run_command(self, command, patch):
> 
> It's not immediately obvious to me what 'patch' is in the context of running
> a command. Ditto to all other occurrences of `patch` in `run_command`.
> 

patch is an Attachment object (maybe we should rename it attachment?). We use "patch" as the name of an argument in other CommitQueueTaskDelegate functions. If we chose to rename this argument then we should rename it in the other CommitQueueTaskDelegate functions. I suggest we do this is a separate bug.

> > Tools/Scripts/webkitpy/tool/commands/queues_unittest.py:89
> > +        teed_stdout = WritableStatusServerStatusFileObject(tool.status_server, queue.name, patch)
> 
> Did you mean to use 'teed' instead of 'tee' here?

I meant to use "teed".

[Daniel Bates]

Created attachment 343580 [details]
Patch and unit tests

[EWS Watchlist]

Attachment 343580 [details] did not pass style-queue:


ERROR: Tools/QueueStatusServer/handlers/updatestatus.py:70:  [UpdateStatus.post] Instance of 'UpdateStatus' has no 'request' member  [pylint/E1101] [5]
ERROR: Tools/QueueStatusServer/handlers/updatestatus.py:72:  [UpdateStatus.post] Instance of 'UpdateStatus' has no 'response' member  [pylint/E1101] [5]
Total errors found: 2 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dean Johnson]

(In reply to Daniel Bates from comment #7)
> (In reply to Dean Johnson from comment #6)
> > Comment on attachment 343111 [details]
> > Patch and unit tests
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=343111&action=review
> > 
> > Overall, the patch looks really good and this functionality will be great to
> > have for EWS. I have some design concerns around storage of / access to logs
> > after they expire from memcached, but those can be addressed in a future
> > patch as this is a large improvement to the existing system.
> > 
> > > Tools/ChangeLog:41
> > > +        streams to the database if a patch fails to build of causes tests failures.
> > 
> > Maybe once the output is finished, we can compress the data and offer it as
> > a gzip stream instead? It seems it would be useful to have full logs if
> > possible. Then those archives could be cleaned up once they are >30 days
> > old, or something similar.
> > 
> 
> Notice that the EWS bot will upload the last 5000 combined characters of
> standard output and standard error for a build or test failure to the status
> server and we persist this data to disk. One example of this can be seen at
> <https://webkit-queues.webkit.org/results/8341523>. We could look to store
> more of output, including the full output, with or without compression.
> Regardless, I suggest we do such an enhancement in a follow up bug.
Okay.
> 
> > > Tools/QueueStatusServer/model/taillog.py:43
> > > +        # Three hours should be a reasonable amount of time to build and test a WebKit change.
> > 
> > Please correct me if I'm wrong here, but this TailLog class will be used to
> > track a single file, such as a build or test log, right? 
> 
> No, we create one TailLog object for entire duration that patch is processed
> by a queue. The underlying channel (crested channel.create_channel()) lives
> for 3 hours (maybe we should increase this limit? maybe use the max limit?).
> The token returned by channel.create_channel() is stored in memcache and
> lives until the queue releases the patch or is evicted due to memory
> pressure or a cache flush, whichever occurs earlier.
So if a patch is processed by mac and iOS builders, then runs through tests, is there only TailLog created to serve streams of all those build and test logs?
> 
> > If so, small nit on
> > 'build and test' -> 'build or test', since this is tracking a single file
> > from a build or test run.
> > 
> 
> Will keep the current wording. See above remark for reason.
> 
> > > Tools/QueueStatusServer/model/taillog.py:59
> > > +        if not memcache.get(key):
> > 
> > Minor nit: This could be simplified by inverting the logic like so:
> > if memcache.get(key):
> >     memchache.delete(key)
> > 
> 
> Actually, it seems sufficient to call memcache.delete() unconditionally
> since memcache makes no lifetime guarantees on the data stored in it. Will
> change the code in this function to read:
> 
> [[
> @classmethod
> def expire(cls, queue_name, attachment_id):
> 	# Deletion is a best effort operation. Memcache may be unavailable or the
> token may have
> 	# been evicted due to memory pressure or an explicit flushing of the cache.
> 	memcache.delete(cls._generate_key(queue_name, attachment_id))
> ]]
> 
> > > Tools/QueueStatusServer/model/taillog.py:64
> > > +        if not memcache.get(self._key):
> > 
> > How are the return values 'True' and 'False' supposed to be used here? I
> > don't see anywhere you use the return value of this function (at least in
> > this patch).
> > 
> 
> The purpose of this code is to check for key existence. As it turns our
> memcache does not have a has(key) API to do this. Towards making this
> clearer I will add the following member function:
> 
>     def _is_valid(self):
>         return memcache.get(self._key) is not None
> 
> And will update the code in this TailLog.write_message() to read:
> 
>     if not self._is_valid():
>         return False
I should have been more explicit about my ask here, sorry. Why does the method 'TailLog.write_message' return True/False, if we don't seem to use the return value anywhere? I see a valid argument to be made for it returning those values, I'm just trying to understand if there's any future plans to use the return value here.
> 
> > Can you also clarify what benefits 'is_transient' offers? As the memcached
> > entry only lasts 3 hours, it feel like any message can already be defined as
> > transient since it will expire in <= 3 hours.
> > 
> > Edit: After reading the commit message I understand a little more of what
> > 'is_transient' means here. I still recommend trying to find a way to improve
> > the wording (or adding a comment) that emphasizes most content is thrown
> > away by default, except for warnings/errors (if I understood correctly).
> > 
> 
> Where do you propose I add such a comment? 
It seems most relevant in 'TailLog.write_message', but could also be placed at the top of the TailLog class.
> 
> > > Tools/QueueStatusServer/templates/patch.html:70
> > > +  }
> > 
> > Is this always true, or only when the memcache entry has expired? 
> 
> Updates are pushed to the channel as they happen and are not stored in
> memory or on disk. So, transient message (used for standard output/standard
> error output) will be lost. Note that we will display all non-transient
> messages on page reload.
I would think that build/test logs are most useful *after* they've been produced since we'll be producing many per patch: Mac build, iOS build, Mac test, iOS test, etc.
> 
> > log information not preserved by memcached upon reload? I would hope someone
> > could access full build logs from a recent run (and ideally from >=3 hours
> > as well).
> > 
> > > Tools/Scripts/webkitpy/common/system/executive.py:129
> > >          if quiet:
> > 
> > For your consideration, I think this logic may read simpler if designed like
> > this:
> > tee_stdout = open(os.devnull, 'w') if quiet else sys.stdout
> 
> I prefer to keep the as-is because of the contextual information afforded by
> the placement of the FIXME comment.
Sounds good.
> 
> > 
> > This does have the downside of not being as explicit in the finally block
> > below when calling `if quiet: tee_stdout.close()`, so I'm not in love with
> > the change and leave it up to you as something to consider.
> > 
> > > Tools/Scripts/webkitpy/common/system/executive.py:133
> > > +            command_output = self.run_with_teed_output_and_throw_if_fail(args, tee_stdout, decode_output=decode_output, **kwargs)
> > 
> > Warning: This isn't recoverable if
> > `self.run_with_teed_output_and_throw_if_fail` raises an exception as there
> > is no except block to allow errors. Is there a case where that may be
> > desirable here?
> > 
> 
> This code was explicitly written so that we let the exception propagate to
> the caller to handle. We only need to take care to do some cleanup hence the
> existence of the finally clause.
> 
> > > Tools/Scripts/webkitpy/common/system/executive.py:139
> > > +    def run_with_teed_output_and_throw_if_fail(self, args, tee_stdout, decode_output, **kwargs):
> > 
> > Nit: throw -> raise. You may also consider replacing `if` with `on`, but I
> > don't feel strongly about it.
> > 
> 
> Will keep as is to match the naming of the exiting function
> Executive._run_command_with_teed_output().
> 
> > > Tools/Scripts/webkitpy/common/system/executive_mock.py:102
> > > +    def run_with_teed_output_and_throw_if_fail(self, args, tee_stdout, quiet=False, cwd=None, env=None):
> > 
> > Did you mean `tee` rather than `teed` in the function name?
> > 
> 
> Will rename tee_stdout to teed_stdout.
> 
> > > Tools/Scripts/webkitpy/common/system/executive_mock.py:104
> > > +            env_string = ""
> > 
> > Nit: For your consideration you could simplify this statement as:
> > `env_string = "" if not env else ", env=%s" % env`
> > 
> > It may also read better using format, but I'm not familiar with the usual
> > paradigms used here so won't recommend one of the other:
> > `env_string = " if not env else ", env={}".format(env)
> > 
> 
> I'll consider it.
> 
> > I also recommend checking for `if env is None` in the compressed if block
> > instead of `if not env` based on previous discussion we've had, but leave it
> > up to you on which paradigm you prefer.
> > 
> 
> Will keep as is. Using `if env is None` will not produce the same result
> when env := {} and I would need to check correctness and/or rebase the
> expected results of at least 16 tests:
Fair enough.
> 
> webkitpy.tool.bot.irc_command_unittest.IRCCommandTest.
> test_rollout_updates_working_copy
> webkitpy.tool.commands.download_unittest.DownloadCommandsTest.
> test_land_cowhand
> webkitpy.tool.commands.download_unittest.DownloadCommandsTest.
> test_apply_watch_list
> webkitpy.tool.commands.download_unittest.DownloadCommandsTest.
> test_check_style
> webkitpy.tool.steps.steps_unittest.StepsTest.test_build_jsc
> webkitpy.tool.steps.steps_unittest.StepsTest.test_runtests_api_debug
> webkitpy.tool.steps.steps_unittest.StepsTest.test_runtests_debug_args
> webkitpy.tool.steps.steps_unittest.StepsTest.test_runtests_api
> webkitpy.tool.steps.steps_unittest.StepsTest.test_runtests_webkitpy
> webkitpy.tool.steps.steps_unittest.StepsTest.test_runtests_bindings
> webkitpy.tool.steps.steps_unittest.StepsTest.test_runtests_jsc_debug
> webkitpy.tool.steps.commit_unittest.CommitTest.test_check_test_expectations
> webkitpy.tool.steps.runtests_unittest.RunTestsTest.test_webkit_run_unit_tests
> webkitpy.tool.steps.steps_unittest.StepsTest.test_build_jsc_debug
> webkitpy.tool.steps.steps_unittest.StepsTest.test_runtests_args
> webkitpy.tool.steps.steps_unittest.StepsTest.test_runtests_jsc
> 
> > > Tools/Scripts/webkitpy/tool/bot/commitqueuetask_unittest.py:52
> > > +    def run_command(self, command, patch):
> > 
> > It's not immediately obvious to me what 'patch' is in the context of running
> > a command. Ditto to all other occurrences of `patch` in `run_command`.
> > 
> 
> patch is an Attachment object (maybe we should rename it attachment?). We
> use "patch" as the name of an argument in other CommitQueueTaskDelegate
> functions. If we chose to rename this argument then we should rename it in
> the other CommitQueueTaskDelegate functions. I suggest we do this is a
> separate bug.
I'm fine with this happening in a separate bug, and I think 'attachment' makes a lot more sense as an uneducated reader.
> 
> > > Tools/Scripts/webkitpy/tool/commands/queues_unittest.py:89
> > > +        teed_stdout = WritableStatusServerStatusFileObject(tool.status_server, queue.name, patch)
> > 
> > Did you mean to use 'teed' instead of 'tee' here?
> 
> I meant to use "teed".

[Aakash Jain]

Can you please post the link or screenshot of how the UI looks like?

How much extra memory does this functionality requires? The server is usually under heavy-memory load, so we might run into problems.

Also, can appscale handle the load properly? For e.g.: when multiple (40+) bots uploads a lots of logs (e.g.: compile logs) to the server at the same time, can appscale handle that properly. 

We have been having "Concurrent transaction exception" on appscale server <rdar://problem/33379545>. We should do good amount of testing to verify that this functionality doesn't make it worse, or introduce other similar issues.

[EWS Watchlist]

Comment on attachment 343580 [details]
Patch and unit tests

Attachment 343580 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8348454

New failing tests:
http/tests/security/contentSecurityPolicy/userAgentShadowDOM/allow-audio.html

[EWS Watchlist]

Created attachment 343622 [details]
Archive of layout-test-results from ews206 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Daniel Bates]

Created attachment 343648 [details]
[Screenshot] Logging

[Daniel Bates]

Created attachment 343649 [details]
Patch and unit tests

[EWS Watchlist]

Attachment 343649 [details] did not pass style-queue:


ERROR: Tools/QueueStatusServer/handlers/updatestatus.py:70:  [UpdateStatus.post] Instance of 'UpdateStatus' has no 'request' member  [pylint/E1101] [5]
ERROR: Tools/QueueStatusServer/handlers/updatestatus.py:72:  [UpdateStatus.post] Instance of 'UpdateStatus' has no 'response' member  [pylint/E1101] [5]
Total errors found: 2 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Daniel Bates]

(In reply to Aakash Jain from comment #11)
> Can you please post the link or screenshot of how the UI looks like?
> 

See attachment #343648 [details]. I am happy to iterate on the UI. I prefer to do so in follow up bugs.

> How much extra memory does this functionality requires? 

I did not measure. I did not see a noticeable increase in memory usage on the AppEngine dashboard though I was only testing with a single patch at a time. Do you mind if we try this change out and see how things go? We can always revert the change should it cause an unexpected hiccup.

> [...]
> 
> Also, can appscale handle the load properly? For e.g.: when multiple (40+)
> bots uploads a lots of logs (e.g.: compile logs) to the server at the same
> time, can appscale handle that properly. 
> 

See above remark.

> We have been having "Concurrent transaction exception" on appscale server
> <rdar://problem/33379545>. We should do good amount of testing to verify
> that this functionality doesn't make it worse, or introduce other similar
> issues.

From my understanding, the Channel API does not interact with a database. And my proposed patch does not interact explicitly interact with a database. So, I am unclear how my patch could cause a "Concurrent transaction exception" or exacerbate the occurrence of such exceptions.

[Ryosuke Niwa]

Comment on attachment 343649 [details]
Patch and unit tests

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

> Tools/ChangeLog:10
> +        Makes the patch status page (e.g. https://webkit-queues.webkit.org/patch/342789/mac-ews) display
> +        real-time status updates, including a tail(1) of the standard output and standard error streams
> +        from the bot as it runs commands to build and test a patch. You can either access the patch status

That would add way too much noise to https://webkit-queues.webkit.org/patch/342789/mac-ews.
How about putting into a separate page?

> Tools/ChangeLog:20
> +        A side effect of this change this that the patch status patch now lists events in chronological

patch status patch -> patch status page?

> Tools/QueueStatusServer/handlers/patch.py:39
> +        statuses = QueueStatus.all().filter("active_patch_id =", attachment_id).order("date")

Nit: AppEngine queries usually don't place a space before/after =.

[David Kilzer (:ddkilzer)]

Comment on attachment 343649 [details]
Patch and unit tests

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

r=me, but need to work out Aakash's memory concerns (how do we verify whether it will create a memory issue?) and design.  I'm okay landing and iterating on the design (as I look at the page rarely), but I *really* like the idea of having more/live status when I do look at the page because it's hard to know whether it's stuck or where it's stuck currently.

>> Tools/ChangeLog:10
>> +        from the bot as it runs commands to build and test a patch. You can either access the patch status
> 
> That would add way too much noise to https://webkit-queues.webkit.org/patch/342789/mac-ews.
> How about putting into a separate page?

What about putting the output behind a "disclosure triangle" for each step such that you only see the output when you open a triangle?

Maybe all steps except the most recent step could be closed by default?  (Maybe that already will happen when a step ends?)

[Daniel Bates]

EWS is being phased out in favor of New EWS, which supports real-time logging. No need to pursue this patch anymore.