Re: Review Request 43358: Added support for all additional Perf Events in PerfEventIsolator.

[Joris Van Remoortere]

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



Closing this review due to inactivity. Please see our 
[guidelines](https://github.com/apache/mesos/blob/master/docs/reopening-reviews.md)
 for reopening reviews.

- Joris Van Remoortere


On Feb. 10, 2016, 4:36 p.m., Bartek Plotka wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43358/
> ---
> 
> (Updated Feb. 10, 2016, 4:36 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Chi Zhang, Ian Downes, Niklas 
> Nielsen, Paul Brett, Szymon Konefal, and Cong Wang.
> 
> 
> Bugs: MESOS-4595
> https://issues.apache.org/jira/browse/MESOS-4595
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added dynamic map `extra_events` for all additional perf events which are not 
> specified explicitly `PerfStatistics`
> TODO in next PRs: Add test and extend description for `--perf_events=` option 
> in Mesos flags.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 194750e92020753e60154083a47bdc3398d31466 
>   src/linux/perf.cpp 1c113a2b3f57877e132bbd65e01fb2f045132128 
> 
> Diff: https://reviews.apache.org/r/43358/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Bartek Plotka
> 
>

Re: Review Request 43358: Added support for all additional Perf Events in PerfEventIsolator.

[haosdent huang]

> On Feb. 10, 2016, 1:41 p.m., Szymon Konefal wrote:
> > src/linux/perf.cpp, line 479
> > 
> >
> > Shouldn't we use 'nullptr' instead of NULL?
> 
> Bartek Plotka wrote:
> +1 IMO `nullptr` sounds like a good choice. All, do you agree with that?

LoL, I remember have a ticket said we should use nullptr instead of NULL. 
https://issues.apache.org/jira/browse/MESOS-3243


- haosdent


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


On Feb. 10, 2016, 12:39 p.m., Bartek Plotka wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43358/
> ---
> 
> (Updated Feb. 10, 2016, 12:39 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Chi Zhang, Ian Downes, Niklas Nielsen, 
> Paul Brett, Szymon Konefal, and Cong Wang.
> 
> 
> Bugs: MESOS-4595
> https://issues.apache.org/jira/browse/MESOS-4595
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added dynamic map `extra_events` for all additional perf events which are not 
> specified explicitly `PerfStatistics`
> TODO in next PRs: Add test and extend description for `--perf_events=` option 
> in Mesos flags.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 194750e92020753e60154083a47bdc3398d31466 
>   src/linux/perf.cpp 1c113a2b3f57877e132bbd65e01fb2f045132128 
> 
> Diff: https://reviews.apache.org/r/43358/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Bartek Plotka
> 
>

Re: Review Request 43358: Added support for all additional Perf Events in PerfEventIsolator.

[Bartek Plotka]

> On Feb. 10, 2016, 1:41 p.m., Szymon Konefal wrote:
> > src/linux/perf.cpp, line 479
> > 
> >
> > Shouldn't we use 'nullptr' instead of NULL?

+1 IMO `nullptr` sounds like a good choice. All, do you agree with that?


- Bartek


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


On Feb. 10, 2016, 12:39 p.m., Bartek Plotka wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43358/
> ---
> 
> (Updated Feb. 10, 2016, 12:39 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Chi Zhang, Ian Downes, Niklas Nielsen, 
> Paul Brett, Szymon Konefal, and Cong Wang.
> 
> 
> Bugs: MESOS-4595
> https://issues.apache.org/jira/browse/MESOS-4595
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added dynamic map `extra_events` for all additional perf events which are not 
> specified explicitly `PerfStatistics`
> TODO in next PRs: Add test and extend description for `--perf_events=` option 
> in Mesos flags.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 194750e92020753e60154083a47bdc3398d31466 
>   src/linux/perf.cpp 1c113a2b3f57877e132bbd65e01fb2f045132128 
> 
> Diff: https://reviews.apache.org/r/43358/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Bartek Plotka
> 
>

Re: Review Request 43358: Added support for all additional Perf Events in PerfEventIsolator.

[Szymon Konefal]

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




src/linux/perf.cpp (line 474)


Shouldn't we use 'nullptr' instead of NULL?


- Szymon Konefal


On Feb. 10, 2016, 12:39 p.m., Bartek Plotka wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43358/
> ---
> 
> (Updated Feb. 10, 2016, 12:39 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Chi Zhang, Ian Downes, Niklas Nielsen, 
> Paul Brett, Szymon Konefal, and Cong Wang.
> 
> 
> Bugs: MESOS-4595
> https://issues.apache.org/jira/browse/MESOS-4595
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added dynamic map `extra_events` for all additional perf events which are not 
> specified explicitly `PerfStatistics`
> TODO in next PRs: Add test and extend description for `--perf_events=` option 
> in Mesos flags.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 194750e92020753e60154083a47bdc3398d31466 
>   src/linux/perf.cpp 1c113a2b3f57877e132bbd65e01fb2f045132128 
> 
> Diff: https://reviews.apache.org/r/43358/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Bartek Plotka
> 
>

Re: Review Request 43358: Added support for all additional Perf Events in PerfEventIsolator.

[Bartek Plotka]

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

(Updated Feb. 10, 2016, 4:36 p.m.)


Review request for mesos, Ben Mahler, Chi Zhang, Ian Downes, Niklas Nielsen, 
Paul Brett, Szymon Konefal, and Cong Wang.


Changes
---

Moved to nullptr


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


Repository: mesos


Description
---

Added dynamic map `extra_events` for all additional perf events which are not 
specified explicitly `PerfStatistics`
TODO in next PRs: Add test and extend description for `--perf_events=` option 
in Mesos flags.


Diffs (updated)
-

  include/mesos/mesos.proto 194750e92020753e60154083a47bdc3398d31466 
  src/linux/perf.cpp 1c113a2b3f57877e132bbd65e01fb2f045132128 

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


Testing
---

make check


Thanks,

Bartek Plotka

Re: Review Request 43358: Added support for all additional Perf Events in PerfEventIsolator.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [43358]

Passed command: export OS='ubuntu:14.04' CONFIGURATION='--verbose' 
COMPILER='gcc' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh

- Mesos ReviewBot


On Feb. 10, 2016, 4:36 p.m., Bartek Plotka wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43358/
> ---
> 
> (Updated Feb. 10, 2016, 4:36 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Chi Zhang, Ian Downes, Niklas Nielsen, 
> Paul Brett, Szymon Konefal, and Cong Wang.
> 
> 
> Bugs: MESOS-4595
> https://issues.apache.org/jira/browse/MESOS-4595
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added dynamic map `extra_events` for all additional perf events which are not 
> specified explicitly `PerfStatistics`
> TODO in next PRs: Add test and extend description for `--perf_events=` option 
> in Mesos flags.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 194750e92020753e60154083a47bdc3398d31466 
>   src/linux/perf.cpp 1c113a2b3f57877e132bbd65e01fb2f045132128 
> 
> Diff: https://reviews.apache.org/r/43358/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Bartek Plotka
> 
>

Re: Review Request 43358: Added support for all additional Perf Events in PerfEventIsolator.

[Bartek Plotka]

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

(Updated Feb. 10, 2016, 12:39 p.m.)


Review request for mesos, Ben Mahler, Chi Zhang, Ian Downes, Niklas Nielsen, 
Paul Brett, Szymon Konefal, and Cong Wang.


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


Repository: mesos


Description (updated)
---

Added dynamic map `extra_events` for all additional perf events which are not 
specified explicitly `PerfStatistics`
TODO in next PRs: Add test and extend description for `--perf_events=` option 
in Mesos flags.


Diffs
-

  include/mesos/mesos.proto 194750e92020753e60154083a47bdc3398d31466 
  src/linux/perf.cpp 1c113a2b3f57877e132bbd65e01fb2f045132128 

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


Testing
---

make check


Thanks,

Bartek Plotka

Re: Review Request 43358: Added support for all additional Perf Events in PerfEventIsolator.

[Bartek Plotka]

> On Feb. 10, 2016, 1:54 a.m., haosdent huang wrote:
> > src/linux/perf.cpp, line 471
> > 
> >
> > How about move `` and `` out in a const 
> > variable? So that avoid we write these strings again and agiain.

+1, will do (:


> On Feb. 10, 2016, 1:54 a.m., haosdent huang wrote:
> > src/linux/perf.cpp, line 493
> > 
> >
> > should be better use `0u` here? Refer to bernd's patch 
> > https://reviews.apache.org/r/43359/diff/1#index_header to fix compile error 
> > in gcc 5.2.

True. What about the line 479? We also should use `0.0` instead of `0` for 
double assignment.


- Bartek


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


On Feb. 9, 2016, 12:54 p.m., Bartek Plotka wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43358/
> ---
> 
> (Updated Feb. 9, 2016, 12:54 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Chi Zhang, Ian Downes, Niklas Nielsen, 
> Paul Brett, Szymon Konefal, and Cong Wang.
> 
> 
> Bugs: MESOS-4595
> https://issues.apache.org/jira/browse/MESOS-4595
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added dynamic map `extra_events` for all additional perf events which are not 
> specified explicitly `PerfStatistics`
> TODO in next PRs: Add test and extend description for `--perf_events=` option 
> in Mesos flags.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 194750e92020753e60154083a47bdc3398d31466 
>   src/linux/perf.cpp 1c113a2b3f57877e132bbd65e01fb2f045132128 
> 
> Diff: https://reviews.apache.org/r/43358/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Bartek Plotka
> 
>

Re: Review Request 43358: Added support for all additional Perf Events in PerfEventIsolator.

[haosdent huang]

> On Feb. 10, 2016, 1:54 a.m., haosdent huang wrote:
> > src/linux/perf.cpp, line 493
> > 
> >
> > should be better use `0u` here? Refer to bernd's patch 
> > https://reviews.apache.org/r/43359/diff/1#index_header to fix compile error 
> > in gcc 5.2.
> 
> Bartek Plotka wrote:
> True. What about the line 479? We also should use `0.0` instead of `0` 
> for double assignment.

+1 for use 0.0 in double assignment.


- haosdent


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


On Feb. 9, 2016, 12:54 p.m., Bartek Plotka wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43358/
> ---
> 
> (Updated Feb. 9, 2016, 12:54 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Chi Zhang, Ian Downes, Niklas Nielsen, 
> Paul Brett, Szymon Konefal, and Cong Wang.
> 
> 
> Bugs: MESOS-4595
> https://issues.apache.org/jira/browse/MESOS-4595
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added dynamic map `extra_events` for all additional perf events which are not 
> specified explicitly `PerfStatistics`
> TODO in next PRs: Add test and extend description for `--perf_events=` option 
> in Mesos flags.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 194750e92020753e60154083a47bdc3398d31466 
>   src/linux/perf.cpp 1c113a2b3f57877e132bbd65e01fb2f045132128 
> 
> Diff: https://reviews.apache.org/r/43358/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Bartek Plotka
> 
>

Re: Review Request 43358: Added support for all additional Perf Events in PerfEventIsolator.

[Bartek Plotka]

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

(Updated Feb. 10, 2016, 12:38 p.m.)


Review request for mesos, Ben Mahler, Chi Zhang, Ian Downes, Niklas Nielsen, 
Paul Brett, Szymon Konefal, and Cong Wang.


Changes
---

Addressed haosdent huang issues.


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


Repository: mesos


Description (updated)
---

Added dynamic map `extra_events` for all additional perf events which are not 
specified explicitly `PerfStatistics`
TODO in next PRs: Add test and extend description in `--perf_events=` option in 
Mesos flags.


Diffs (updated)
-

  include/mesos/mesos.proto 194750e92020753e60154083a47bdc3398d31466 
  src/linux/perf.cpp 1c113a2b3f57877e132bbd65e01fb2f045132128 

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


Testing
---

make check


Thanks,

Bartek Plotka

Re: Review Request 43358: Added support for all additional Perf Events in PerfEventIsolator.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [43358]

Passed command: export OS='ubuntu:14.04' CONFIGURATION='--verbose' 
COMPILER='gcc' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh

- Mesos ReviewBot


On Feb. 9, 2016, 12:54 p.m., Bartek Plotka wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43358/
> ---
> 
> (Updated Feb. 9, 2016, 12:54 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Chi Zhang, Ian Downes, Niklas Nielsen, 
> Paul Brett, Szymon Konefal, and Cong Wang.
> 
> 
> Bugs: MESOS-4595
> https://issues.apache.org/jira/browse/MESOS-4595
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added dynamic map `extra_events` for all additional perf events which are not 
> specified explicitly `PerfStatistics`
> TODO in next PRs: Add test and extend description for `--perf_events=` option 
> in Mesos flags.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 194750e92020753e60154083a47bdc3398d31466 
>   src/linux/perf.cpp 1c113a2b3f57877e132bbd65e01fb2f045132128 
> 
> Diff: https://reviews.apache.org/r/43358/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Bartek Plotka
> 
>

Re: Review Request 43358: Added support for all additional Perf Events in PerfEventIsolator.

[Bartek Plotka]

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

(Updated Feb. 9, 2016, 12:54 p.m.)


Review request for mesos, Ben Mahler, Chi Zhang, Ian Downes, Niklas Nielsen, 
Paul Brett, Szymon Konefal, and Cong Wang.


Changes
---

Fixed small style issues.


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


Repository: mesos


Description (updated)
---

Added dynamic map `extra_events` for all additional perf events which are not 
specified explicitly `PerfStatistics`
TODO in next PRs: Add test and extend description for `--perf_events=` option 
in Mesos flags.


Diffs (updated)
-

  include/mesos/mesos.proto 194750e92020753e60154083a47bdc3398d31466 
  src/linux/perf.cpp 1c113a2b3f57877e132bbd65e01fb2f045132128 

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


Testing (updated)
---

make check


Thanks,

Bartek Plotka

Re: Review Request 43358: Added support for all additional Perf Events in PerfEventIsolator.

[Bartek Plotka]

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

(Updated Feb. 9, 2016, 12:47 p.m.)


Review request for mesos, Ben Mahler, Chi Zhang, Ian Downes, Niklas Nielsen, 
Paul Brett, Szymon Konefal, and Cong Wang.


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


Repository: mesos


Description (updated)
---

Added dynamic map extra_events for all additional perf events which are not 
specified explicitly in PerfStatistics


Diffs
-

  include/mesos/mesos.proto 194750e92020753e60154083a47bdc3398d31466 
  src/linux/perf.cpp 1c113a2b3f57877e132bbd65e01fb2f045132128 

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


Testing
---


Thanks,

Bartek Plotka

Re: Review Request 43358: Added support for all additional Perf Events in PerfEventIsolator.

[Bartek Plotka]

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

(Updated Feb. 9, 2016, 12:47 p.m.)


Review request for mesos, Ben Mahler, Chi Zhang, Ian Downes, Niklas Nielsen, 
Paul Brett, Szymon Konefal, and Cong Wang.


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


Repository: mesos


Description (updated)
---

Signed-off-by: Bartlomiej Plotka 


Diffs (updated)
-

  include/mesos/mesos.proto 194750e92020753e60154083a47bdc3398d31466 
  src/linux/perf.cpp 1c113a2b3f57877e132bbd65e01fb2f045132128 

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


Testing
---


Thanks,

Bartek Plotka

Review Request 43358: Added support for all additional Perf Events in PerfEventIsolator.

[Bartek Plotka]

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

Review request for mesos, Ben Mahler, Chi Zhang, Ian Downes, Niklas Nielsen, 
Paul Brett, Szymon Konefal, and Cong Wang.


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


Repository: mesos


Description
---

Added dynamic map `extra_events` for all additional perf events which are not 
specified explicitly in `PerfStatistics`

TODO in next RRs: Unit test & extra description in `--perf_event=` mesos slave 
option.


Diffs
-

  include/mesos/mesos.proto 194750e92020753e60154083a47bdc3398d31466 
  src/linux/perf.cpp 1c113a2b3f57877e132bbd65e01fb2f045132128 

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


Testing
---


Thanks,

Bartek Plotka

Re: Review Request 43358: Added support for all additional Perf Events in PerfEventIsolator.

[haosdent huang]

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


Fix it, then Ship it!




Ship It!


src/linux/perf.cpp (line 466)


How about move `` and `` out in a const 
variable? So that avoid we write these strings again and agiain.



src/linux/perf.cpp (line 488)


should be better use `0u` here? Refer to bernd's patch 
https://reviews.apache.org/r/43359/diff/1#index_header to fix compile error in 
gcc 5.2.


- haosdent huang


On Feb. 9, 2016, 12:54 p.m., Bartek Plotka wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43358/
> ---
> 
> (Updated Feb. 9, 2016, 12:54 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Chi Zhang, Ian Downes, Niklas Nielsen, 
> Paul Brett, Szymon Konefal, and Cong Wang.
> 
> 
> Bugs: MESOS-4595
> https://issues.apache.org/jira/browse/MESOS-4595
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added dynamic map `extra_events` for all additional perf events which are not 
> specified explicitly `PerfStatistics`
> TODO in next PRs: Add test and extend description for `--perf_events=` option 
> in Mesos flags.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 194750e92020753e60154083a47bdc3398d31466 
>   src/linux/perf.cpp 1c113a2b3f57877e132bbd65e01fb2f045132128 
> 
> Diff: https://reviews.apache.org/r/43358/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Bartek Plotka
> 
>