mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benno Evers <bev...@mesosphere.com>
Subject Re: Review Request 69064: Added unit tests for Master HTTP endpoints.
Date Wed, 19 Dec 2018 12:08:27 GMT


> On Dec. 18, 2018, 8:32 a.m., Greg Mann wrote:
> > src/tests/master_load_tests.cpp
> > Lines 555 (patched)
> > <https://reviews.apache.org/r/69064/diff/4/?file=2113358#file2113358line555>
> >
> >     Can you use some other header to test this case, so that we don't need to disable
the test? You could even set an arbitrary header which is not interpreted by libprocess.
> 
> Benno Evers wrote:
>     The endpoints themselves will ignore all headers, so the idea of this test was to
verify that libprocess can still correctly handle the headers that libprocess cares about.
So using another header which is ignored by libprocess will not really work for that purpose.
>     
>     I suppose we could drop the test completely, but I think the chances are pretty low
that we will remember (and allocate time) to write this test from scratch in the future, when
it becomes possible to enable it.
> 
> Greg Mann wrote:
>     Could you elaborate on the reason for the test? I don't quite understand why we need
a test to verify that libprocess can still correctly handle some headers? I was thinking that
the test was trying to verify that de-duplication was working correctly in cases where requests
differ only in the headers that they specify, but it doesn't look like that's what the body
of the test is doing.

Let me try to elaborate:

Right now, we get a different response when requesting a gzipped vs. a plain text response:
(with 'response' I'm referring to the data that's sent over the wire, not the decoded content,
which also answers your question below):

```
$ echo -e "GET /state HTTP/1.1\nAccept-Encoding: gzip\r\n" | nc localhost 5050 | xxd
00000000: 4854 5450 2f31 2e31 2032 3030 204f 4b0d  HTTP/1.1 200 OK.
00000010: 0a43 6f6e 7465 6e74 2d45 6e63 6f64 696e  .Content-Encodin
00000020: 673a 2067 7a69 700d 0a44 6174 653a 2057  g: gzip..Date: W
00000030: 6564 2c20 3139 2044 6563 2032 3031 3820  ed, 19 Dec 2018 
00000040: 3130 3a30 323a 3433 2047 4d54 0d0a 436f  10:02:43 GMT..Co
00000050: 6e74 656e 742d 5479 7065 3a20 6170 706c  ntent-Type: appl
00000060: 6963 6174 696f 6e2f 6a73 6f6e 0d0a 436f  ication/json..Co
00000070: 6e74 656e 742d 4c65 6e67 7468 3a20 3739  ntent-Length: 79
00000080: 330d 0a0d 0a1f 8b08 0000 0000 0000 03ad  3...............
00000090: 55cb 8ed3 3014 fd15 6489 5d1f 491a cf84  U...0...d.].I...
[...]

$ echo -e "GET /state HTTP/1.1\nAccept-Encoding: identity\r\n" | nc localhost 5050 | xxd
00000000: 4854 5450 2f31 2e31 2032 3030 204f 4b0d  HTTP/1.1 200 OK.
00000010: 0a44 6174 653a 2057 6564 2c20 3139 2044  .Date: Wed, 19 D
00000020: 6563 2032 3031 3820 3130 3a30 333a 3034  ec 2018 10:03:04
00000030: 2047 4d54 0d0a 436f 6e74 656e 742d 5479   GMT..Content-Ty
00000040: 7065 3a20 6170 706c 6963 6174 696f 6e2f  pe: application/
00000050: 6a73 6f6e 0d0a 436f 6e74 656e 742d 4c65  json..Content-Le
00000060: 6e67 7468 3a20 3138 3935 0d0a 0d0a 7b22  ngth: 1895....{"
00000070: 7665 7273 696f 6e22 3a22 312e 362e 3022  version":"1.6.0"
00000080: 2c22 6275 696c 645f 6461 7465 223a 2232  ,"build_date":"2
00000090: 3031 382d 3035 2d30 3420 3230 3a33 343a  018-05-04 20:34:
[...]
```

If this were to change due to the request being batched together with another one, we'd probably
consider it to be a bug, and the idea of having this test is to ensure this will not accidentally
break in the future.

Technically, we could also move this test to libprocess and just verify that two HTTP responses
created from the same future will get encoded independently, but that test would also need
to be `DISABLED` right now for the same reason, i.e. libprocess not being able to actually
receive the gzip-encoded response.

Also, thinking about it, clients currently can't depend on response compression anyways due
to MESOS-9451/MESOS-9453, so we could probably also just accept the current state of affairs
and drop this test case completely.


- Benno


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69064/#review211377
-----------------------------------------------------------


On Dec. 18, 2018, 6:41 p.m., Benno Evers wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69064/
> -----------------------------------------------------------
> 
> (Updated Dec. 18, 2018, 6:41 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This commit adds a set of unit test to verify that
> basic Master HTTP endpoints still work correctly
> under the presence of request caching.
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am 7a4904a3d67479267087fd2313a263d8218843fa 
>   src/tests/master_load_tests.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/69064/diff/5/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Benno Evers
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message