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 2020/07/16 14:20:45 UTC
[GitHub] [geode-native] alb3rtobr opened a new pull request #629: GEODE-8364: Change log level at runtime
alb3rtobr opened a new pull request #629:
URL: https://github.com/apache/geode-native/pull/629
----------------------------------------------------------------
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 #629: GEODE-8364: Change log level at runtime
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #629:
URL: https://github.com/apache/geode-native/pull/629#discussion_r456530395
##########
File path: cppcache/include/geode/Cache.hpp
##########
@@ -239,6 +241,10 @@ class APACHE_GEODE_EXPORT Cache : public GeodeCache {
SystemProperties& getSystemProperties() const override;
+ void setLogLevel(LogLevel newLevel);
Review comment:
Please add documentation to these new methods.
##########
File path: cppcache/test/CacheTest.cpp
##########
@@ -61,3 +62,10 @@ TEST(CacheTest, close) {
EXPECT_THROW(cache.readyForEvents(), CacheClosedException);
EXPECT_THROW(cache.rootRegions(), CacheClosedException);
}
+
+TEST(CacheTest, changeLogLevel) {
+ auto cache = CacheFactory{}.set("log-level", "info").create();
+ ASSERT_EQ(cache.getLogLevel(), LogLevel::Info);
Review comment:
What happens if you call `cache.getSystemProperties.getLogLevel()` after changing it via `Cache`?
----------------------------------------------------------------
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 #629: GEODE-8364: Change log level at runtime
Posted by GitBox <gi...@apache.org>.
pivotal-jbarrett commented on a change in pull request #629:
URL: https://github.com/apache/geode-native/pull/629#discussion_r456531368
##########
File path: cppcache/test/CacheTest.cpp
##########
@@ -61,3 +62,10 @@ TEST(CacheTest, close) {
EXPECT_THROW(cache.readyForEvents(), CacheClosedException);
EXPECT_THROW(cache.rootRegions(), CacheClosedException);
}
+
+TEST(CacheTest, changeLogLevel) {
+ auto cache = CacheFactory{}.set("log-level", "info").create();
+ ASSERT_EQ(cache.getLogLevel(), LogLevel::Info);
Review comment:
What happens if you call `cache.getSystemProperties().getLogLevel()` after changing it via `Cache`?
----------------------------------------------------------------
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] alb3rtobr commented on pull request #629: GEODE-8364: Change log level at runtime
Posted by GitBox <gi...@apache.org>.
alb3rtobr commented on pull request #629:
URL: https://github.com/apache/geode-native/pull/629#issuecomment-659983218
It will be useful for us to have this feature in order to get more info when there is a problem with the client. If we detect an issue in the client, we can activate debug log to get more info about what is happening. This will help a lot our engineers to troubleshoot and report problems.
----------------------------------------------------------------
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 commented on pull request #629: GEODE-8364: Change log level at runtime
Posted by GitBox <gi...@apache.org>.
pdxcodemonkey commented on pull request #629:
URL: https://github.com/apache/geode-native/pull/629#issuecomment-659489989
Do you have a use case for this? I don't suppose it's really harming anything, but I'm not sure I understand why/how you'd want to change this midstream, and it could cause problems down the road for things like log analysis tools. We've also been planning for a long time to completely replace our logging mechanism with an off-the-shelf solution like spdlog or log4cpp, and this would add a new logging feature that we'd have to replicate with any such change.
----------------------------------------------------------------
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] alb3rtobr commented on a change in pull request #629: GEODE-8364: Change log level at runtime
Posted by GitBox <gi...@apache.org>.
alb3rtobr commented on a change in pull request #629:
URL: https://github.com/apache/geode-native/pull/629#discussion_r457982380
##########
File path: cppcache/include/geode/Cache.hpp
##########
@@ -239,6 +241,10 @@ class APACHE_GEODE_EXPORT Cache : public GeodeCache {
SystemProperties& getSystemProperties() const override;
+ void setLogLevel(LogLevel newLevel);
Review comment:
done
----------------------------------------------------------------
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] alb3rtobr commented on pull request #629: GEODE-8364: Change log level at runtime
Posted by GitBox <gi...@apache.org>.
alb3rtobr commented on pull request #629:
URL: https://github.com/apache/geode-native/pull/629#issuecomment-671786298
@pivotal-jbarrett could you please check if the changes are ok for you? Thanks
----------------------------------------------------------------
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] alb3rtobr commented on a change in pull request #629: GEODE-8364: Change log level at runtime
Posted by GitBox <gi...@apache.org>.
alb3rtobr commented on a change in pull request #629:
URL: https://github.com/apache/geode-native/pull/629#discussion_r457982869
##########
File path: cppcache/test/CacheTest.cpp
##########
@@ -61,3 +62,10 @@ TEST(CacheTest, close) {
EXPECT_THROW(cache.readyForEvents(), CacheClosedException);
EXPECT_THROW(cache.rootRegions(), CacheClosedException);
}
+
+TEST(CacheTest, changeLogLevel) {
+ auto cache = CacheFactory{}.set("log-level", "info").create();
+ ASSERT_EQ(cache.getLogLevel(), LogLevel::Info);
Review comment:
I have changed the `setLogLevel` method to also update the log level on the `SystemProperties` object.
----------------------------------------------------------------
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 commented on a change in pull request #629: GEODE-8364: Change log level at runtime
Posted by GitBox <gi...@apache.org>.
pdxcodemonkey commented on a change in pull request #629:
URL: https://github.com/apache/geode-native/pull/629#discussion_r456634323
##########
File path: cppcache/test/CacheTest.cpp
##########
@@ -61,3 +62,10 @@ TEST(CacheTest, close) {
EXPECT_THROW(cache.readyForEvents(), CacheClosedException);
EXPECT_THROW(cache.rootRegions(), CacheClosedException);
}
+
+TEST(CacheTest, changeLogLevel) {
+ auto cache = CacheFactory{}.set("log-level", "info").create();
+ ASSERT_EQ(cache.getLogLevel(), LogLevel::Info);
Review comment:
Exactly - I was thinking about this earlier this morning. We can still get into the kind of weird situation where the `geode.properties` file says `log-level` is one value but in fact it's another, and the `SystemProperties` object is also in disagreement about `log-level`. I don't think the first situation is worth worrying about - unless I'm very mistaken, you'll be in this state even when you set `log-level` on the cache factory to a custom value. The second, however, it seems to me we should be able to prevent.
----------------------------------------------------------------
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] mkevo merged pull request #629: GEODE-8364: Change log level at runtime
Posted by GitBox <gi...@apache.org>.
mkevo merged pull request #629:
URL: https://github.com/apache/geode-native/pull/629
----------------------------------------------------------------
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