mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Greg Mann <g...@mesosphere.io>
Subject Re: Review Request 61262: Added 'heartbeat' event for the operator API.
Date Tue, 15 Aug 2017 18:44:02 GMT

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




src/master/master.hpp
Lines 309-311 (patched)
<https://reviews.apache.org/r/61262/#comment258950>

    Now that we're sending heartbeats for multiple purposes, I'm concerned that the logging
will be ambiguous. Instead of templating this class on the ID type, perhaps it makes more
sense to initialize it with a string that it can use for logging purposes. For example:
    
    ```
    class Heartbeater : public process::Process<Heartbeater>
    {
    public:
      Heartbeater(const string& _message,
                  const HttpConnection& _http,
                  const Duration& _interval,
                  const Duration& _delay = Seconds(0))
        : process::ProcessBase(process::ID::generate("heartbeater")),
          message(_message),
          http(_http),
          interval(_interval),
          delay(_delay) {}
    
    ...
    
    private:
      void heartbeat()
      {
        // Only send a heartbeat if the connection is not closed.
        if (http.closed().isPending()) {
          VLOG(1) << "Sending heartbeat to " << message;
          
          etc...
    ```
    
    and then for the framework case we would do something like:
    ```
    heartbeater = new Heartbeater<FrameworkID>(
        "framework " + stringify(info.id()),
        http.get(),
        DEFAULT_HEARTBEAT_INTERVAL);
    ```
    
    and the subscriber case something like:
    ```
    heartbeater = new Heartbeater<UUID>(
        "subscriber " + stringify(http.streamId),
        http,
        DEFAULT_HEARTBEAT_INTERVAL,
        DEFAULT_HEARTBEAT_INTERVAL);
    ```
    
    what do you think?



src/tests/api_tests.cpp
Lines 2381 (patched)
<https://reviews.apache.org/r/61262/#comment258960>

    How about:
    "Verifies that 'HEARTBEAT' events are sent at the correct times."



src/tests/api_tests.cpp
Lines 2444 (patched)
<https://reviews.apache.org/r/61262/#comment258961>

    You should also verify that the type of the event is correct here.


- Greg Mann


On Aug. 14, 2017, 9:51 p.m., Quinn Leng wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61262/
> -----------------------------------------------------------
> 
> (Updated Aug. 14, 2017, 9:51 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Greg Mann.
> 
> 
> Bugs: MESOS-7695
>     https://issues.apache.org/jira/browse/MESOS-7695
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added the 'HEARTBEAT' event for the operator API, modified other
> related test cases to accept heartbeats.
> 
> 
> Diffs
> -----
> 
>   include/mesos/master/master.proto fc5bd894ce55fe8e946d4c5b4b33d3c0505f3c2b 
>   include/mesos/v1/master/master.proto c3fb31de2509adcdec8204f8bbe46b46a31540e8 
>   src/master/http.cpp 959091c8ec03b6ac7bcb5d21b04d2f7d5aff7d54 
>   src/master/master.hpp b802fd153a10f6012cea381f153c28cc78cae995 
>   src/tests/api_tests.cpp 3ab4740bcac29ecb89585da6adb1f563d6fc1f5f 
> 
> 
> Diff: https://reviews.apache.org/r/61262/diff/5/
> 
> 
> Testing
> -------
> 
> make check -j48
> 
> 
> Thanks,
> 
> Quinn Leng
> 
>


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