Re: Review Request 40806: Made HDFS::exists asynchronous.

[Jie Yu]

> On Dec. 4, 2015, 10:51 a.m., Bernd Mathiske wrote:
> > src/hdfs/hdfs.cpp, line 143
> > 
> >
> > Maybe we can get some extra info about what subprocess failed to be 
> > reaped by capturing some values from above in the closure?

Hum, this is pretty standard in our code base. I don't think adding the command 
itself to the log will help debugging a lot. Let me know if you insist.


- Jie


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


On Dec. 4, 2015, 12:45 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40806/
> ---
> 
> (Updated Dec. 4, 2015, 12:45 a.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Ben Mahler, and Vinod Kone.
> 
> 
> Bugs: MESOS-3951
> https://issues.apache.org/jira/browse/MESOS-3951
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Made HDFS::exists asynchronous.
> 
> This also fixed a bug in the orignal code: the original code never returns 
> false.
> 
> 
> Diffs
> -
> 
>   src/cli/execute.cpp ac933b6a8636af3356f3d6a99656c0185077c262 
>   src/hdfs/hdfs.hpp 6bdeedf8934a681b32fca8edc251488f4213dd07 
>   src/hdfs/hdfs.cpp c32c2ae9d2ace5deb09f56dcec2bd366c2d07f6b 
> 
> Diff: https://reviews.apache.org/r/40806/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 40806: Made HDFS::exists asynchronous.

[Bernd Mathiske]

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

Ship it!



src/hdfs/hdfs.cpp (line 143)


Maybe we can get some extra info about what subprocess failed to be reaped 
by capturing some values from above in the closure?


- Bernd Mathiske


On Dec. 3, 2015, 4:45 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40806/
> ---
> 
> (Updated Dec. 3, 2015, 4:45 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Ben Mahler, and Vinod Kone.
> 
> 
> Bugs: MESOS-3951
> https://issues.apache.org/jira/browse/MESOS-3951
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Made HDFS::exists asynchronous.
> 
> This also fixed a bug in the orignal code: the original code never returns 
> false.
> 
> 
> Diffs
> -
> 
>   src/cli/execute.cpp ac933b6a8636af3356f3d6a99656c0185077c262 
>   src/hdfs/hdfs.hpp 6bdeedf8934a681b32fca8edc251488f4213dd07 
>   src/hdfs/hdfs.cpp c32c2ae9d2ace5deb09f56dcec2bd366c2d07f6b 
> 
> Diff: https://reviews.apache.org/r/40806/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 40806: Made HDFS::exists asynchronous.

[Jie Yu]

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

(Updated Dec. 4, 2015, 12:45 a.m.)


Review request for mesos, Bernd Mathiske, Ben Mahler, and Vinod Kone.


Bugs: MESOS-3951
https://issues.apache.org/jira/browse/MESOS-3951


Repository: mesos


Description
---

Made HDFS::exists asynchronous.

This also fixed a bug in the orignal code: the original code never returns 
false.


Diffs
-

  src/cli/execute.cpp ac933b6a8636af3356f3d6a99656c0185077c262 
  src/hdfs/hdfs.hpp 6bdeedf8934a681b32fca8edc251488f4213dd07 
  src/hdfs/hdfs.cpp c32c2ae9d2ace5deb09f56dcec2bd366c2d07f6b 

Diff: https://reviews.apache.org/r/40806/diff/


Testing
---

make check


Thanks,

Jie Yu

Re: Review Request 40806: Made HDFS::exists asynchronous.

[Jie Yu]

> On Dec. 1, 2015, 10:40 a.m., Bernd Mathiske wrote:
> > src/hdfs/hdfs.cpp, line 100
> > 
> >
> > This block seems to be reusable in a lot of situations. Any chance we 
> > could abstract it and make it part of class Subprocess?
> > 
> > In a separate ticket, I suppose. ==> Place a TODO here?

Due to some controversy in the Subprocess patch. I move the logic to hdfs.cpp 
as a helper and that slightly simplies the logics here.


- Jie


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


On Dec. 4, 2015, 12:20 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40806/
> ---
> 
> (Updated Dec. 4, 2015, 12:20 a.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Ben Mahler, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Made HDFS::exists asynchronous.
> 
> This also fixed a bug in the orignal code: the original code never returns 
> false.
> 
> 
> Diffs
> -
> 
>   src/cli/execute.cpp ac933b6a8636af3356f3d6a99656c0185077c262 
>   src/hdfs/hdfs.hpp 6bdeedf8934a681b32fca8edc251488f4213dd07 
>   src/hdfs/hdfs.cpp c32c2ae9d2ace5deb09f56dcec2bd366c2d07f6b 
> 
> Diff: https://reviews.apache.org/r/40806/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 40806: Made HDFS::exists asynchronous.

[Jie Yu]

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

(Updated Dec. 4, 2015, 12:20 a.m.)


Review request for mesos, Bernd Mathiske, Ben Mahler, and Vinod Kone.


Changes
---

Address review comments.


Repository: mesos


Description
---

Made HDFS::exists asynchronous.

This also fixed a bug in the orignal code: the original code never returns 
false.


Diffs (updated)
-

  src/cli/execute.cpp ac933b6a8636af3356f3d6a99656c0185077c262 
  src/hdfs/hdfs.hpp 6bdeedf8934a681b32fca8edc251488f4213dd07 
  src/hdfs/hdfs.cpp c32c2ae9d2ace5deb09f56dcec2bd366c2d07f6b 

Diff: https://reviews.apache.org/r/40806/diff/


Testing
---

make check


Thanks,

Jie Yu

Re: Review Request 40806: Made HDFS::exists asynchronous.

[Jie Yu]

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

(Updated Dec. 4, 2015, 12:09 a.m.)


Review request for mesos, Bernd Mathiske, Ben Mahler, and Vinod Kone.


Changes
---

Rebased.


Repository: mesos


Description
---

Made HDFS::exists asynchronous.

This also fixed a bug in the orignal code: the original code never returns 
false.


Diffs (updated)
-

  src/cli/execute.cpp ac933b6a8636af3356f3d6a99656c0185077c262 
  src/hdfs/hdfs.hpp 6bdeedf8934a681b32fca8edc251488f4213dd07 
  src/hdfs/hdfs.cpp c32c2ae9d2ace5deb09f56dcec2bd366c2d07f6b 

Diff: https://reviews.apache.org/r/40806/diff/


Testing
---

make check


Thanks,

Jie Yu

Re: Review Request 40806: Made HDFS::exists asynchronous.

[Jie Yu]

> On Dec. 3, 2015, 10:42 p.m., Vinod Kone wrote:
> > i'm assuming there are already tests that verify this change?

just checked. Not every function. I'll add tests in the subsequent reviews.


- Jie


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


On Dec. 1, 2015, 1:21 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40806/
> ---
> 
> (Updated Dec. 1, 2015, 1:21 a.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Ben Mahler, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Made HDFS::exists asynchronous.
> 
> This also fixed a bug in the orignal code: the original code never returns 
> false.
> 
> 
> Diffs
> -
> 
>   src/cli/execute.cpp ac933b6a8636af3356f3d6a99656c0185077c262 
>   src/hdfs/hdfs.hpp 6bdeedf8934a681b32fca8edc251488f4213dd07 
>   src/hdfs/hdfs.cpp c32c2ae9d2ace5deb09f56dcec2bd366c2d07f6b 
> 
> Diff: https://reviews.apache.org/r/40806/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 40806: Made HDFS::exists asynchronous.

[Vinod Kone]

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

Ship it!


i'm assuming there are already tests that verify this change?


src/hdfs/hdfs.cpp (line 103)


maybe prepend a comma here?



src/hdfs/hdfs.cpp (line 106)


ditto. prepend a comma here?


- Vinod Kone


On Dec. 1, 2015, 1:21 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40806/
> ---
> 
> (Updated Dec. 1, 2015, 1:21 a.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Ben Mahler, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Made HDFS::exists asynchronous.
> 
> This also fixed a bug in the orignal code: the original code never returns 
> false.
> 
> 
> Diffs
> -
> 
>   src/cli/execute.cpp ac933b6a8636af3356f3d6a99656c0185077c262 
>   src/hdfs/hdfs.hpp 6bdeedf8934a681b32fca8edc251488f4213dd07 
>   src/hdfs/hdfs.cpp c32c2ae9d2ace5deb09f56dcec2bd366c2d07f6b 
> 
> Diff: https://reviews.apache.org/r/40806/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 40806: Made HDFS::exists asynchronous.

[Bernd Mathiske]

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



src/hdfs/hdfs.cpp (line 82)


s/exec/execute/

'to exec' seems to indicate that the exec syscall failed. That may not 
always be the cause for the failure.



src/hdfs/hdfs.cpp (line 100)


This block seems to be reusable in a lot of situations. Any chance we could 
abstract it and make it part of class Subprocess?

In a separate ticket, I suppose. ==> Place a TODO here?


- Bernd Mathiske


On Nov. 30, 2015, 5:21 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40806/
> ---
> 
> (Updated Nov. 30, 2015, 5:21 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Ben Mahler, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Made HDFS::exists asynchronous.
> 
> This also fixed a bug in the orignal code: the original code never returns 
> false.
> 
> 
> Diffs
> -
> 
>   src/cli/execute.cpp ac933b6a8636af3356f3d6a99656c0185077c262 
>   src/hdfs/hdfs.hpp 6bdeedf8934a681b32fca8edc251488f4213dd07 
>   src/hdfs/hdfs.cpp c32c2ae9d2ace5deb09f56dcec2bd366c2d07f6b 
> 
> Diff: https://reviews.apache.org/r/40806/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 40806: Made HDFS::exists asynchronous.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [40250, 40251, 40252, 40274, 40253, 40305, 40403, 40418, 
40556, 40461, 40462, 40463, 40464, 40498, 40559, 40806]

Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export 
COMPILER=gcc; ./support/docker_build.sh

- Mesos ReviewBot


On Dec. 1, 2015, 1:21 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40806/
> ---
> 
> (Updated Dec. 1, 2015, 1:21 a.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Ben Mahler, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Made HDFS::exists asynchronous.
> 
> This also fixed a bug in the orignal code: the original code never returns 
> false.
> 
> 
> Diffs
> -
> 
>   src/cli/execute.cpp ac933b6a8636af3356f3d6a99656c0185077c262 
>   src/hdfs/hdfs.hpp 6bdeedf8934a681b32fca8edc251488f4213dd07 
>   src/hdfs/hdfs.cpp c32c2ae9d2ace5deb09f56dcec2bd366c2d07f6b 
> 
> Diff: https://reviews.apache.org/r/40806/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Review Request 40806: Made HDFS::exists asynchronous.

[Jie Yu]

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

Review request for mesos, Bernd Mathiske, Ben Mahler, and Vinod Kone.


Repository: mesos


Description
---

Made HDFS::exists asynchronous.

This also fixed a bug in the orignal code: the original code never returns 
false.


Diffs
-

  src/cli/execute.cpp ac933b6a8636af3356f3d6a99656c0185077c262 
  src/hdfs/hdfs.hpp 6bdeedf8934a681b32fca8edc251488f4213dd07 
  src/hdfs/hdfs.cpp c32c2ae9d2ace5deb09f56dcec2bd366c2d07f6b 

Diff: https://reviews.apache.org/r/40806/diff/


Testing
---

make check


Thanks,

Jie Yu