-
Notifications
You must be signed in to change notification settings - Fork 476
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
issues #413 : Debug mode which allows HTTP requests being sent to the… #450
Conversation
…ent to the database to be logged
Codecov Report
@@ Coverage Diff @@
## master #450 +/- ##
============================================
+ Coverage 86.48% 86.55% +0.07%
Complexity 301 301
============================================
Files 20 20
Lines 1287 1294 +7
Branches 134 135 +1
============================================
+ Hits 1113 1120 +7
Misses 114 114
Partials 60 60
Continue to review full report at Codecov.
|
logLevels.forEach(logLevel -> { | ||
System.out.println("LogLevel = " + logLevel); | ||
Optional.ofNullable(logLevel).ifPresent(value -> { | ||
System.setProperty(InfluxDB.LOG_LEVEL_PROP, value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it's a good idea to change global configurations (in this case, defining global properties) during the test as this may affect other tests if they are running in parallel. Also, if your test fail here for some reason, you will exit the test case (line 38: fail(e)) and may leave the system with an incorrect level configuration.
My example may be too much for this simple test but my point here is to make you aware that this is, in general, a bad practice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add try/finally block
For now I see the Surefire plugin config runs tests in sequential. however add NotThreadSafe annotation to make sure Surefire will execute the test sequentially
http://maven.apache.org/surefire/maven-surefire-plugin/examples/fork-options-and-parallel-execution.html
@@ -739,4 +739,28 @@ public void dropRetentionPolicy(final String rpName, final String database) { | |||
execute(this.influxDBService.postQuery(this.username, this.password, | |||
Query.encode(queryBuilder.toString()))); | |||
} | |||
|
|||
private void setLogLevel() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- This method is being called every time an object is created. Can we call it just once, statically?
- IMO the name of this method is not following the JavaBean convention:
setXXX
usually is used to change the state and expects 1..* parameter(s). Can you rename it to something likeconfigureLogLevel
? - I'm almost sure all this method can be replaced with something like:
String value = System.getProperty...
if (value != null) {
// logLevel = ... don't change this property with this method as there is already another method changing it.
// instead, call directly setLogLevel(...)
setLogLevel(LogLevel.valueOf(value.toUpperCase()));
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method is being called every time an object is created. Can we call it just once, statically?
I dont think so, I think we should allow developer to change the property value on the fly (as you can see in the unit test) for debugging purpose, or its visibility may differ from context to context (custom SecurityManager is used)
IMO the name of this method is not following the JavaBean convention: setXXX usually is used to change the state and expects 1..* parameter(s). Can you rename it to something like configureLogLevel?
For me the naming convention setXXX(...) and setXXX() are not semantically ambiguous. But rename it to initLogLevel anyway
I'm almost sure all this method can be replaced with something like
partially agree and fix accordingly, except keeping switch case since you still have check for null or invalid property value (either by conditional structure like this or catching exception from valueOf(String))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dont think so, I think we should allow developer to change the property value on the fly (as you can see in the unit test) for debugging purpose
So, you want to provide a way to the developer to change the loglevel of InfluxDB objects already instantiated in order to make the debug easier. Is this correct? If so, then you expect the developer to create on his side something like what you did on the test case: System.setProperty
. How do you expect that the developer will change the loglevel while the application is running?
If I got your intentions correctly and what I said before is right then I suggest you to do the right way and use JMX.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For me the naming convention setXXX(...) and setXXX() are not semantically ambiguous. But rename it to initLogLevel anyway
Please check the Section "8.3 Design Patterns for Properties" from the JavaBean API specification: http://download.oracle.com/otn-pub/jcp/7224-javabeans-1.01-fr-spec-oth-JSpec/beans.101.pdf .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
partially agree and fix accordingly, except keeping switch case since you still have check for null or invalid property value (either by conditional structure like this or catching exception from valueOf(String))
Ok., I don't think you got my point of view. Let me be more clear: If you have 100 different loglevels, would you still keep using a switch/case statement? AFAIK, your code can be replaced and "secured" with a try/catch with no collateral effects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, you want to provide a way to the developer to change the loglevel of InfluxDB objects already instantiated in order to make the debug easier. Is this correct? If so, then you expect the developer to create on his side something like what you did on the test case: System.setProperty. How do you expect that the developer will change the loglevel while the application is running?
If I got your intentions correctly and what I said before is right then I suggest you to do the right way and use JMX.
As far as I know JMX is a technology for managing and monitoring rather than debugging the application. For debugging you would like to use a more flexible but simple way, and use of a System property here would be better. Two use cases:
1/ You can pass the log level to JVM at booting up
2/ During your debugging session, you can manage to set the log level of the client which is going to created somewhere else (I do not reject you can use JMX but it will never have such a flexibility like evaluating a java statement to change that System property value)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please check the Section "8.3 Design Patterns for Properties" from the JavaBean API specification: http://download.oracle.com/otn-pub/jcp/7224-javabeans-1.01-fr-spec-oth-JSpec/beans.101.pdf .
I've had this already, so initLogLevel() is ok for you or not ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok., I don't think you got my point of view. Let me be more clear: If you have 100 different loglevels, would you still keep using a switch/case statement? AFAIK, your code can be replaced and "secured" with a try/catch with no collateral effects.
OK, got it, fixed as your comment (use of Enum.valueOf(,))
Tks
* | ||
*/ | ||
@RunWith(JUnitPlatform.class) | ||
@NotThreadSafe |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you 100% sure this annotation will affect how JUnit works?
Anyway, instead of fixing the bad practice you introduced by making changes that may affect other tests you are here trying to do something that would make the tests not going in parallel? I don't get it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update: I found later on your email the link for the specific configuration for the surefire plugin, so it will work.
But my question from the previous comment is still valid.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I follow the Surefire plugin document, please check
http://maven.apache.org/surefire/maven-surefire-plugin/examples/fork-options-and-parallel-execution.html
For tests that may change the global context, I think making it isolated and run in sequential is quite reasonable. If I've got you right, that means every test must be able to run in parallel ?
change to use Enum.valueOf(,) as reviewing comment
logLevels.forEach(logLevel -> { | ||
System.out.println("LogLevel = " + logLevel); | ||
Optional.ofNullable(logLevel).ifPresent(value -> { | ||
System.setProperty(InfluxDB.LOG_LEVEL_PROP, value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi,
I went through the code with the following findings:
-
InfluxDB.LOG_LEVEL_PROP ? It should be InfluxDB.LOG_LEVEL_PROPERTY. It is 5 chars more only.
-
I don't even like the 'initLogLevel' construct. Rather we should do:
this.setLogLevel(this.getConfiguredLogLevel())
getConfiguredLogLevel() would just read the configuration property and set a default value for the LogLevel. Like this, we have two functions where it is clear what is happening here and there this one function less that has side effects.
3) If the testing is a problem we may even do (simplified without error processing):
this.setLogLevel(this.getConfiguredLogLevel(System.getProperty(InfluxDB.LOG_LEVEL_PROPERTY)))
In this case, we may test the getConfiguredLogLevel
function individually. (If we agree that we don't need to test System.getProperty) And we would avoid the discussion about forcing sequential test execution.
Vlada
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just repeating myself: I went through the code with the following findings: InfluxDB.LOG_LEVEL_PROP ? It should be InfluxDB.LOG_LEVEL_PROPERTY. It is 5 chars more only. I don't even like the 'initLogLevel' construct. Rather we should do: this.setLogLevel(this.getConfiguredLogLevel()) getConfiguredLogLevel() would just read the configuration property and set a default value for the LogLevel. Like this, we have two functions where it is clear what is happening here and there this one function less that has side effects. this.setLogLevel(this.getConfiguredLogLevel(System.getProperty(InfluxDB.LOG_LEVEL_PROPERTY))) In this case, we may test the getConfiguredLogLevel function individually. (If we agree that we don't need to test System.getProperty) And we would avoid the discussion about forcing sequential test execution. Vlada |
@dubsky fine by me and just to be clear: my point was not about testing or not something specific. It was originally about changing state(s) that are globally visible and may or may not affect concurrent tests. Something else that I mentioned is regarding changing some configurations at runtime by calling Yes, I agree that will work in some cases but it's not standardized and the developer will have to find a way to trigger this
I can solve all previous issues using JMX but not with [1] ehcache http://www.ehcache.org/documentation/2.7/operations/jmx.html @dubsky @majst01 @ivankudibal what do you think? |
Just want to quote here this (from a discussion between @fmachado and me)
So unless we change the original issue to something like Operation management and monitoring mechanism for influxdb-java ... , I would keep on with System property. There are thousands of real practices we can find easily on the Internet for using of System property to configure certain behaviour(s) of an application. Hence, I think I do not need to list some of them here. However I agree with that I will remove the Use Case /2. So in case if this system property exists, any change to it's value will not take effect and we can set the logLevel in a static context which is executed only one time. I hope this change is aligned with the review
|
8871d01
to
75b6ada
Compare
+ value change of debug mode logLevel is not supported + LOG_LEVEL_PROPERTY instead of LOG_LEVEL_PROP
@@ -59,6 +59,7 @@ | |||
static final okhttp3.MediaType MEDIA_TYPE_STRING = MediaType.parse("text/plain"); | |||
|
|||
private static final String SHOW_DATABASE_COMMAND_ENCODED = Query.encode("SHOW DATABASES"); | |||
private static final String DEBUG_MODE_LOG_LEVEL = System.getProperty(LOG_LEVEL_PROPERTY); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
small nit, why DEBUG_MODE_LOG_LEVEL ? would not LOG_LEVEL be more accurate ?
I am fine with this, my single comment regarding property naming might be of interest, but whats really missing is a small hint in the documentation and a CHANGELOG.md entry. Thanks |
+ source code documentation of changelog entry
@majst01 , I added change log and source documentation, thank you |
… database to be logged
this PR is to fix #413