> On Jan. 14, 2016, 10:33 p.m., Vinod Kone wrote: > > src/executor/executor.cpp, line 123 > > <https://reviews.apache.org/r/41283/diff/7/?file=1193992#file1193992line123> > > > > The name of this struct is weird considering it is encapsulating two > > connections. > > > > s/HttpConnection/connections/ ? > > > > Also, do we really need a struct?
Dropped this `struct` and instead used two members for the `Connection` object called `subscribe`/`nonSubscribe`. > On Jan. 14, 2016, 10:33 p.m., Vinod Kone wrote: > > src/executor/executor.cpp, lines 179-183 > > <https://reviews.apache.org/r/41283/diff/7/?file=1193992#file1193992line179> > > > > is mesos_slave_pid the only way an executor is going to know about the > > http endpoint to connect? i think we need to have a better env variable > > because 'pid' is a relic of the old world, which doesn't make sense in the > > http world. > > > > wasn't there an MESOS_AGENT_ENDPOINT? Yep, there is. But we need `MESOS_SLAVE_PID` for testing since we don't have delegates set for `slave(X)` -> root i.e. `/api/v1/executor`. As per our discussion, I would file a separate JIRA for this since the scheduler also uses the `PID` from the master currently. > On Jan. 14, 2016, 10:33 p.m., Vinod Kone wrote: > > src/executor/executor.cpp, lines 282-287 > > <https://reviews.apache.org/r/41283/diff/7/?file=1193992#file1193992line282> > > > > This definitely needs a comment here. IIUC, you are opening up two > > connections here one for subscribe and another for everything else. > > > > More importantly, why are you opening both connections here? I would've > > expected to only open the subscribe connection if it's a subscribe calla > > and open a non-subscribe one if/when it's a nonsubscribe call. Fixed. Now we create the `Subscribe` connection initially and invoke the `connected` callback thereafter. We only create the second connection on a need basis and don't invoke the `disconnected` callback if it breaks. The `disconnected` callback is only invoked for the `Subscribe` connection. > On Jan. 14, 2016, 10:33 p.m., Vinod Kone wrote: > > src/executor/executor.cpp, line 334 > > <https://reviews.apache.org/r/41283/diff/7/?file=1193992#file1193992line334> > > > > why do we wait for `recoveryTimeout` if the agent is disconnected and > > we are not checkpointing? I was initially relying on `recoveryTimeout` to be `0` initially. Changed it to be an `Option`. > On Jan. 14, 2016, 10:33 p.m., Vinod Kone wrote: > > src/executor/executor.cpp, line 382 > > <https://reviews.apache.org/r/41283/diff/7/?file=1193992#file1193992line382> > > > > s/been/seen/ ? This wasn't clear to me. This comment is similar to the already existing one in `src/exec/exec.cpp`. Feel free to reopen. - Anand ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/41283/#review114253 ----------------------------------------------------------- On Jan. 20, 2016, 10:11 p.m., Anand Mazumdar wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/41283/ > ----------------------------------------------------------- > > (Updated Jan. 20, 2016, 10:11 p.m.) > > > Review request for mesos, Ben Mahler and Vinod Kone. > > > Bugs: MESOS-3550 > https://issues.apache.org/jira/browse/MESOS-3550 > > > Repository: mesos > > > Description > ------- > > This change introduces the implementation for the executor library. > > This uses the new HTTP Connection interface to ensure calls are properly > pipelined. > > `connected` -> Callback invoked when a TCP connection is established with the > agent for the `Subscribe` call. > `disconnected` -> When the `Subscribe` TCP connection is interrupted possibly > due to an agent restart. > `received` -> When the executor receives events from the agent upon > subscribing. > > > Diffs > ----- > > src/Makefile.am d23e35001078a86775bd9b76baa207ecb9dab7e1 > src/executor/executor.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/41283/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Anand Mazumdar > >
