[GitHub] jmeter issue #254: Feature/bz60589-1 PR for review (logkit to slf4j / log4j2...

2017-02-05 Thread pmouawad
Github user pmouawad commented on the issue:

https://github.com/apache/jmeter/pull/254
  
Forget about my comment on jcl


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---


[GitHub] jmeter issue #254: Feature/bz60589-1 PR for review (logkit to slf4j / log4j2...

2017-02-05 Thread pmouawad
Github user pmouawad commented on the issue:

https://github.com/apache/jmeter/pull/254
  
I'm not sure commons-logging can be dropped as it's a compile dependency of 
HttpClient 4.5.3:

https://repo1.maven.org/maven2/org/apache/httpcomponents/httpclient/4.5.3/httpclient-4.5.3.pom


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---


[GitHub] jmeter issue #254: Feature/bz60589-1 PR for review (logkit to slf4j / log4j2...

2017-01-30 Thread woonsan
Github user woonsan commented on the issue:

https://github.com/apache/jmeter/pull/254
  
I've fixed more:
- Keep the backward compatibility in the Help / En(Dis)able DEBUG logging 
menu items.
- Fixed errors in HttpMirrorServer (executed via mirror-server.sh or 
mirror-server.bat). It now uses log4j. Also, it provides log level setting 
through command line arguments (e.g, `-LDEBUG' or `-Ljmeter=DEBUG 
-Ljorphan=DEBUG'). This command line arguments for log level settings are 
provided to replace the old log level setting functionality through .properties 
file loading). It supports `-P' to set port number command line arg as well 
as the backward compatible the first port number arg (e.g, ./mirror-server.sh 
).
- Fixed a corner case when GuiLogAppender is used in log4j2.xml while 
JMeter application properties were not initialized properly.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---


[GitHub] jmeter issue #254: Feature/bz60589-1 PR for review (logkit to slf4j / log4j2...

2017-01-26 Thread woonsan
Github user woonsan commented on the issue:

https://github.com/apache/jmeter/pull/254
  
@pmouawad , thanks a lot for the quick review!
Regarding NewDriver#replaceDateFormatInFileName(), here's the reason why I 
added that:
- As LoggingManager#makeWriter() was gone 
(https://github.com/apache/jmeter/blob/trunk/src/jorphan/org/apache/jorphan/logging/LoggingManager.java#L174),
 I needed to have the same feature somewhere else to keep the backward 
compatibility for the log file name option with date format 
(http://jmeter.apache.org/usermanual/get-started.html#options)
- However, the date format in the log file name option has been broken for 
long time in my understanding. For example, if I pass 
"jmeter_'MMddHHmmss'.log" option value, it simply fails because 
SimpleDateFormat complains about unrecognized format character, 'j', for 
instance.
- Therefore, I rewrote the LoggingManager#makeWriter() method in 
NewDriver#replaceDateFormatInFileName() in order to extract string inside 
single quote pair and format / append the value using SimpleDateFormat, not the 
whole (which leads formatting failure always, resulting log file name option 
used literally, not evaluated with current date).


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---


[GitHub] jmeter issue #254: Feature/bz60589-1 PR for review (logkit to slf4j / log4j2...

2017-01-26 Thread pmouawad
Github user pmouawad commented on the issue:

https://github.com/apache/jmeter/pull/254
  
Hello ,
Thanks for this big piece of work.
I made a first review, it looks good to me.
There's one thing I don't clearly understand, what's the use of 
replaceDateFormatInFileName in NewDriver ?

Thanks


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---


[GitHub] jmeter issue #254: Feature/bz60589-1 PR for review (logkit to slf4j / log4j2...

2017-01-26 Thread woonsan
Github user woonsan commented on the issue:

https://github.com/apache/jmeter/pull/254
  
Now, I think this PR is ready for review (bugzilla: 
https://bz.apache.org/bugzilla/show_bug.cgi?id=60589).
Please take a review.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---