You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Alex Clemmer <cl...@gmail.com> on 2016/12/03 00:06:36 UTC
Re: Review Request 53550: Rename symbols in log.proto to avoid naming
collision in win32 API.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53550/
-----------------------------------------------------------
(Updated Dec. 3, 2016, 12:06 a.m.)
Review request for mesos, Daniel Pravat and Joseph Wu.
Repository: mesos
Description
-------
The standard win32 headers define a macro, `IGNORE`, which presently
collides with two uses of the same symbol in log.proto. This causes a
compile error on Windows.
In this commit, we rename the symbol in the log.proto files. There are
two primary reasons for this.
The first is because the trick we have previously applied to get around
similar problems (in which we `#undef` the macro, and re-define as a
global constant) is made somewaht more complex by the fact that the
log.proto headers are generated by protocol buffers. To implement this
effectively, we'd have to individually `#undef` at every site we include
log.pb.h, which is not a huge deal given the number of #include sites,
but doesn't protect us against future build breaks.
The second is that the approach of re-naming the symbol in log.proto is
not very invasive: we need to change a handful of places where the
system is used, and we never have to think of the issue again. And,
because it is an internal API, we don't require a major version bump to
implement the change.
Diffs (updated)
-----
src/log/consensus.cpp 94ddf245c07ccd38d4fe828bc47c98ecf540243f
src/log/coordinator.cpp 72ef0366633b4e4137fd303fa49f9efb166c80c9
src/log/replica.cpp d596e617d4b4eab91245e0a88a3b9479fc75b813
src/messages/log.proto 6a2c26be2afe411e6927cddebcfd9634b2b1b884
src/tests/log_tests.cpp 99954388eb0fad2acde0cedfd7daa3c9379bfb03
Diff: https://reviews.apache.org/r/53550/diff/
Testing
-------
Thanks,
Alex Clemmer
Re: Review Request 53550: Rename symbols in log.proto to avoid naming
collision in win32 API.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53550/#review158440
-----------------------------------------------------------
Ship it!
This will not cause any compatibility issues as the enumeration is unchanged. When these protobufs are sent over the wire, they are transmitted as byte code (not JSON), which does not include the name of each enum value. As such, this will not break between major versions of the master either.
- Joseph Wu
On Dec. 6, 2016, 6:16 p.m., Alex Clemmer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53550/
> -----------------------------------------------------------
>
> (Updated Dec. 6, 2016, 6:16 p.m.)
>
>
> Review request for mesos, Daniel Pravat and Joseph Wu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The standard win32 headers define a macro, `IGNORE`, which presently
> collides with two uses of the same symbol in log.proto. This causes a
> compile error on Windows.
>
> In this commit, we rename the symbol in the log.proto files. There are
> two primary reasons for this.
>
> The first is because the trick we have previously applied to get around
> similar problems (in which we `#undef` the macro, and re-define as a
> global constant) is made somewaht more complex by the fact that the
> log.proto headers are generated by protocol buffers. To implement this
> effectively, we'd have to individually `#undef` at every site we include
> log.pb.h, which is not a huge deal given the number of #include sites,
> but doesn't protect us against future build breaks.
>
> The second is that the approach of re-naming the symbol in log.proto is
> not very invasive: we need to change a handful of places where the
> system is used, and we never have to think of the issue again. And,
> because it is an internal API, we don't require a major version bump to
> implement the change.
>
>
> Diffs
> -----
>
> src/log/consensus.cpp 94ddf245c07ccd38d4fe828bc47c98ecf540243f
> src/log/coordinator.cpp 72ef0366633b4e4137fd303fa49f9efb166c80c9
> src/log/replica.cpp d596e617d4b4eab91245e0a88a3b9479fc75b813
> src/messages/log.proto 6a2c26be2afe411e6927cddebcfd9634b2b1b884
> src/tests/log_tests.cpp 99954388eb0fad2acde0cedfd7daa3c9379bfb03
>
> Diff: https://reviews.apache.org/r/53550/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alex Clemmer
>
>
Re: Review Request 53550: Rename symbols in log.proto to avoid naming
collision in win32 API.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53550/#review158376
-----------------------------------------------------------
Ship it!
Having spent a fair portion of yesterday dealing with name collisions due to Windows headers, I believe this approach, when it can be taken, should be. Attempting to `#undef` the conflict can be very hairy.
- Andrew Schwartzmeyer
On Dec. 7, 2016, 2:16 a.m., Alex Clemmer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53550/
> -----------------------------------------------------------
>
> (Updated Dec. 7, 2016, 2:16 a.m.)
>
>
> Review request for mesos, Daniel Pravat and Joseph Wu.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The standard win32 headers define a macro, `IGNORE`, which presently
> collides with two uses of the same symbol in log.proto. This causes a
> compile error on Windows.
>
> In this commit, we rename the symbol in the log.proto files. There are
> two primary reasons for this.
>
> The first is because the trick we have previously applied to get around
> similar problems (in which we `#undef` the macro, and re-define as a
> global constant) is made somewaht more complex by the fact that the
> log.proto headers are generated by protocol buffers. To implement this
> effectively, we'd have to individually `#undef` at every site we include
> log.pb.h, which is not a huge deal given the number of #include sites,
> but doesn't protect us against future build breaks.
>
> The second is that the approach of re-naming the symbol in log.proto is
> not very invasive: we need to change a handful of places where the
> system is used, and we never have to think of the issue again. And,
> because it is an internal API, we don't require a major version bump to
> implement the change.
>
>
> Diffs
> -----
>
> src/log/consensus.cpp 94ddf245c07ccd38d4fe828bc47c98ecf540243f
> src/log/coordinator.cpp 72ef0366633b4e4137fd303fa49f9efb166c80c9
> src/log/replica.cpp d596e617d4b4eab91245e0a88a3b9479fc75b813
> src/messages/log.proto 6a2c26be2afe411e6927cddebcfd9634b2b1b884
> src/tests/log_tests.cpp 99954388eb0fad2acde0cedfd7daa3c9379bfb03
>
> Diff: https://reviews.apache.org/r/53550/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alex Clemmer
>
>
Re: Review Request 53550: Rename symbols in log.proto to avoid naming
collision in win32 API.
Posted by Alex Clemmer <cl...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53550/
-----------------------------------------------------------
(Updated Dec. 7, 2016, 2:16 a.m.)
Review request for mesos, Daniel Pravat and Joseph Wu.
Changes
-------
Changed `IGNORE_RESPONSE` -> `IGNORED` and `IGNORE_PROMISE` -> `IGNORED`.
Repository: mesos
Description
-------
The standard win32 headers define a macro, `IGNORE`, which presently
collides with two uses of the same symbol in log.proto. This causes a
compile error on Windows.
In this commit, we rename the symbol in the log.proto files. There are
two primary reasons for this.
The first is because the trick we have previously applied to get around
similar problems (in which we `#undef` the macro, and re-define as a
global constant) is made somewaht more complex by the fact that the
log.proto headers are generated by protocol buffers. To implement this
effectively, we'd have to individually `#undef` at every site we include
log.pb.h, which is not a huge deal given the number of #include sites,
but doesn't protect us against future build breaks.
The second is that the approach of re-naming the symbol in log.proto is
not very invasive: we need to change a handful of places where the
system is used, and we never have to think of the issue again. And,
because it is an internal API, we don't require a major version bump to
implement the change.
Diffs (updated)
-----
src/log/consensus.cpp 94ddf245c07ccd38d4fe828bc47c98ecf540243f
src/log/coordinator.cpp 72ef0366633b4e4137fd303fa49f9efb166c80c9
src/log/replica.cpp d596e617d4b4eab91245e0a88a3b9479fc75b813
src/messages/log.proto 6a2c26be2afe411e6927cddebcfd9634b2b1b884
src/tests/log_tests.cpp 99954388eb0fad2acde0cedfd7daa3c9379bfb03
Diff: https://reviews.apache.org/r/53550/diff/
Testing
-------
Thanks,
Alex Clemmer