You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by GitBox <gi...@apache.org> on 2021/03/23 08:54:38 UTC
[GitHub] [geode-native] gaussianrecurrence opened a new pull request #774: GEODE-9058: Remove ACE_OS references
gaussianrecurrence opened a new pull request #774:
URL: https://github.com/apache/geode-native/pull/774
- All references to ACE_OS have been removed in the source and replaced
by a suitable boost alternative, an STL alternative or in few cases,
a custom alternative.
- Note that most of the impacts are in the old integration tests.
- IMPORTANT. There are two cases in which ACE_OS references remain:
* ExpiryTaskManager. This reference is left there as there is
already an ongoing PR to replace the whole implementation by
boost::asio one. PR #678
* DistributedSystemImpl. There is a call to uname, which I've been
unable to find a suitable alternative. The approach on this needs
to be discussed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] pdxcodemonkey merged pull request #774: GEODE-9058: Remove ACE_OS references
Posted by GitBox <gi...@apache.org>.
pdxcodemonkey merged pull request #774:
URL: https://github.com/apache/geode-native/pull/774
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] pivotal-jbarrett commented on pull request #774: GEODE-9058: Remove ACE_OS references
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on pull request #774:
URL: https://github.com/apache/geode-native/pull/774#issuecomment-805939887
I you have have mostly successful in removing ACE from the old integration tests then keep going but I don't feel like it is a priority. Having ACE as a dependency for that framework doesn't keep me awake at night or keep us from making meaningful progress in the main sources and new test framework.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] gaussianrecurrence commented on a change in pull request #774: GEODE-9058: Remove ACE_OS references
Posted by GitBox <gi...@apache.org>.
gaussianrecurrence commented on a change in pull request #774:
URL: https://github.com/apache/geode-native/pull/774#discussion_r600620699
##########
File path: cppcache/src/DistributedSystemImpl.cpp
##########
@@ -81,9 +81,9 @@ void DistributedSystemImpl::logSystemInformation() const {
ACE_utsname u;
ACE_OS::uname(&u);
- LOGCONFIG(
- "Running on: SystemName=%s Machine=%s Host=%s Release=%s Version=%s",
- u.sysname, u.machine, u.nodename, u.release, u.version);
+ LOGCONFIG("Running on: SystemName=" BOOST_PLATFORM
Review comment:
Thanks for pointing that out. Well as you say. Writing our own solution seems like the only way :S
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] gaussianrecurrence commented on a change in pull request #774: GEODE-9058: Remove ACE_OS references
Posted by GitBox <gi...@apache.org>.
gaussianrecurrence commented on a change in pull request #774:
URL: https://github.com/apache/geode-native/pull/774#discussion_r600752498
##########
File path: cppcache/src/DistributedSystemImpl.cpp
##########
@@ -81,9 +81,9 @@ void DistributedSystemImpl::logSystemInformation() const {
ACE_utsname u;
ACE_OS::uname(&u);
- LOGCONFIG(
- "Running on: SystemName=%s Machine=%s Host=%s Release=%s Version=%s",
- u.sysname, u.machine, u.nodename, u.release, u.version);
+ LOGCONFIG("Running on: SystemName=" BOOST_PLATFORM
Review comment:
That's understandable. Yes, I was planning on re-using ACE implementation, so I'll stick to that!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] gaussianrecurrence commented on a change in pull request #774: GEODE-9058: Remove ACE_OS references
Posted by GitBox <gi...@apache.org>.
gaussianrecurrence commented on a change in pull request #774:
URL: https://github.com/apache/geode-native/pull/774#discussion_r600665284
##########
File path: cppcache/src/DistributedSystemImpl.cpp
##########
@@ -81,9 +81,9 @@ void DistributedSystemImpl::logSystemInformation() const {
ACE_utsname u;
ACE_OS::uname(&u);
- LOGCONFIG(
- "Running on: SystemName=%s Machine=%s Host=%s Release=%s Version=%s",
- u.sysname, u.machine, u.nodename, u.release, u.version);
+ LOGCONFIG("Running on: SystemName=" BOOST_PLATFORM
Review comment:
You've got a point. I did not want to change the output, but it's true that this kind of info is not much relevant and if needed for troubleshooting it can be asked to the issue reporter. If you are OK with removing the call it would make my life a whole lot more easy and future code more maintable. What you think @pivotal-jbarrett ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] gaussianrecurrence commented on pull request #774: GEODE-9058: Remove ACE_OS references
Posted by GitBox <gi...@apache.org>.
gaussianrecurrence commented on pull request #774:
URL: https://github.com/apache/geode-native/pull/774#issuecomment-805945945
> I you have have mostly successful in removing ACE from the old integration tests then keep going but I don't feel like it is a priority. Having ACE as a dependency for that framework doesn't keep me awake at night or keep us from making meaningful progress in the main sources and new test framework.
I am quite close to remove all references. I am currently solving Windows segmentations. Once solved, solving uname and few minors comments will be the remainings.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] pivotal-jbarrett commented on a change in pull request #774: GEODE-9058: Remove ACE_OS references
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #774:
URL: https://github.com/apache/geode-native/pull/774#discussion_r602516382
##########
File path: cppcache/src/DistributedSystemImpl.cpp
##########
@@ -81,9 +81,9 @@ void DistributedSystemImpl::logSystemInformation() const {
ACE_utsname u;
ACE_OS::uname(&u);
- LOGCONFIG(
- "Running on: SystemName=%s Machine=%s Host=%s Release=%s Version=%s",
- u.sysname, u.machine, u.nodename, u.release, u.version);
+ LOGCONFIG("Running on: SystemName=" BOOST_PLATFORM
Review comment:
Well, considering those are 32bit OSs, out of support and not compatible with our minimum runtime, I think we are ok!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] pivotal-jbarrett commented on a change in pull request #774: GEODE-9058: Remove ACE_OS references
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #774:
URL: https://github.com/apache/geode-native/pull/774#discussion_r600746320
##########
File path: cppcache/src/DistributedSystemImpl.cpp
##########
@@ -81,9 +81,9 @@ void DistributedSystemImpl::logSystemInformation() const {
ACE_utsname u;
ACE_OS::uname(&u);
- LOGCONFIG(
- "Running on: SystemName=%s Machine=%s Host=%s Release=%s Version=%s",
- u.sysname, u.machine, u.nodename, u.release, u.version);
+ LOGCONFIG("Running on: SystemName=" BOOST_PLATFORM
Review comment:
It is not uncommon for the person collecting the logs to have little knowledge of the host details so I suspect support staff will not be happy if the are working with a customer who can't tell them what OS and version a log came from. In general I agree though that this is ephemeral information. It is also pretty easy to create the platform independent. You can look to the ACE implementation for hints on how to do the same.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] pivotal-jbarrett commented on a change in pull request #774: GEODE-9058: Remove ACE_OS references
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #774:
URL: https://github.com/apache/geode-native/pull/774#discussion_r600617196
##########
File path: cppcache/src/DistributedSystemImpl.cpp
##########
@@ -81,9 +81,9 @@ void DistributedSystemImpl::logSystemInformation() const {
ACE_utsname u;
ACE_OS::uname(&u);
- LOGCONFIG(
- "Running on: SystemName=%s Machine=%s Host=%s Release=%s Version=%s",
- u.sysname, u.machine, u.nodename, u.release, u.version);
+ LOGCONFIG("Running on: SystemName=" BOOST_PLATFORM
Review comment:
A really important distinction here is that `BOOST_PLATFORM` is resolved at compile time, while `ACE_OS::name` is resolved at runtime. What are you thinking for the other components here? Roll our own system independent solution like `ACE_OS::uname`? When I looked into this I couldn't find a different path.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode-native] gaussianrecurrence commented on a change in pull request #774: GEODE-9058: Remove ACE_OS references
Posted by GitBox <gi...@apache.org>.
gaussianrecurrence commented on a change in pull request #774:
URL: https://github.com/apache/geode-native/pull/774#discussion_r601845671
##########
File path: cppcache/src/DistributedSystemImpl.cpp
##########
@@ -81,9 +81,9 @@ void DistributedSystemImpl::logSystemInformation() const {
ACE_utsname u;
ACE_OS::uname(&u);
- LOGCONFIG(
- "Running on: SystemName=%s Machine=%s Host=%s Release=%s Version=%s",
- u.sysname, u.machine, u.nodename, u.release, u.version);
+ LOGCONFIG("Running on: SystemName=" BOOST_PLATFORM
Review comment:
There it is. I've written an implementation for at least all the supported platforms. I've left out some old Windows versions, like Windows 95/98/Me, but I really doubt someone manages to run geode on those OS.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org