You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@jmeter.apache.org by sebb <se...@gmail.com> on 2012/10/29 01:54:26 UTC
Re: svn commit: r1402574 - /jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http/sampler/HTTPSamplerBase.java
On 26 October 2012 18:04, <mi...@apache.org> wrote:
> Author: milamber
> Date: Fri Oct 26 17:04:00 2012
> New Revision: 1402574
>
> URL: http://svn.apache.org/viewvc?rev=1402574&view=rev
> Log:
> When used the options Retrieve All Embedded Resources + Concurrent pool, these log.warn() generates a lot of lines in jmeter.log.
>
> Modified:
> jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http/sampler/HTTPSamplerBase.java
-1, see below.
> Modified: jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http/sampler/HTTPSamplerBase.java
> URL: http://svn.apache.org/viewvc/jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http/sampler/HTTPSamplerBase.java?rev=1402574&r1=1402573&r2=1402574&view=diff
> ==============================================================================
> --- jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http/sampler/HTTPSamplerBase.java (original)
> +++ jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http/sampler/HTTPSamplerBase.java Fri Oct 26 17:04:00 2012
> @@ -753,7 +753,7 @@ public abstract class HTTPSamplerBase ex
>
> public void setAuthManager(AuthManager value) {
> AuthManager mgr = getAuthManager();
> - if (mgr != null) {
> + if (log.isDebugEnabled() && mgr != null) {
> log.warn("Existing AuthManager " + mgr.getName() + " superseded by " + value.getName());
I'm not very happy with this fix, as it suppresses the warnings when
Concurrent pool is not in use.
It also presumably does not display valid warnings if concurrent pool is in use.
I've not looked at the code yet, but there has to be a better way to do this.
Ideally change the pool code so it does not generate the overrides.
Also, the code looks odd - why check for debug yet report a warning?
> }
> setProperty(new TestElementProperty(AUTH_MANAGER, value));
> @@ -783,7 +783,7 @@ public abstract class HTTPSamplerBase ex
>
> public void setCookieManager(CookieManager value) {
> CookieManager mgr = getCookieManager();
> - if (mgr != null) {
> + if (log.isDebugEnabled() && mgr != null) {
> log.warn("Existing CookieManager " + mgr.getName() + " superseded by " + value.getName());
> }
> setProperty(new TestElementProperty(COOKIE_MANAGER, value));
> @@ -795,7 +795,7 @@ public abstract class HTTPSamplerBase ex
>
> public void setCacheManager(CacheManager value) {
> CacheManager mgr = getCacheManager();
> - if (mgr != null) {
> + if (log.isDebugEnabled() && mgr != null) {
> log.warn("Existing CacheManager " + mgr.getName() + " superseded by " + value.getName());
> }
> setProperty(new TestElementProperty(CACHE_MANAGER, value));
>
>
Re: svn commit: r1402574 - /jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http/sampler/HTTPSamplerBase.java
Posted by sebb <se...@gmail.com>.
On 1 November 2012 14:23, sebb <se...@gmail.com> wrote:
> On 1 November 2012 11:59, sebb <se...@gmail.com> wrote:
>> On 29 October 2012 08:16, Milamber <mi...@apache.org> wrote:
>>>
>>>
>>> Le 29/10/2012 00:54, sebb a ecrit :
>>>>
>>>> On 26 October 2012 18:04,<mi...@apache.org> wrote:
>>>>>
>>>>> Author: milamber
>>>>> Date: Fri Oct 26 17:04:00 2012
>>>>> New Revision: 1402574
>>>>>
>>>>> URL: http://svn.apache.org/viewvc?rev=1402574&view=rev
>>>>> Log:
>>>>> When used the options Retrieve All Embedded Resources + Concurrent pool,
>>>>> these log.warn() generates a lot of lines in jmeter.log.
>>>>>
>>>>> Modified:
>>>>>
>>>>> jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http/sampler/HTTPSamplerBase.java
>>>>
>>>> -1, see below.
>>>>
>>>>> Modified:
>>>>> jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http/sampler/HTTPSamplerBase.java
>>>>> URL:
>>>>> http://svn.apache.org/viewvc/jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http/sampler/HTTPSamplerBase.java?rev=1402574&r1=1402573&r2=1402574&view=diff
>>>>>
>>>>> ==============================================================================
>>>>> ---
>>>>> jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http/sampler/HTTPSamplerBase.java
>>>>> (original)
>>>>> +++
>>>>> jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http/sampler/HTTPSamplerBase.java
>>>>> Fri Oct 26 17:04:00 2012
>>>>> @@ -753,7 +753,7 @@ public abstract class HTTPSamplerBase ex
>>>>>
>>>>> public void setAuthManager(AuthManager value) {
>>>>> AuthManager mgr = getAuthManager();
>>>>> - if (mgr != null) {
>>>>> + if (log.isDebugEnabled()&& mgr != null) {
>>>>>
>>>>> log.warn("Existing AuthManager " + mgr.getName() + "
>>>>> superseded by " + value.getName());
>>>>
>>>> I'm not very happy with this fix, as it suppresses the warnings when
>>>> Concurrent pool is not in use.
>>>> It also presumably does not display valid warnings if concurrent pool is
>>>> in use.
>>>>
>>>> I've not looked at the code yet, but there has to be a better way to do
>>>> this.
>>>> Ideally change the pool code so it does not generate the overrides.
>>>
>>>
>>> In attachment a patch to fix the first incorrect fix.
>>> * revert on setAuthManager(), it's not necessary
>>> * add 2 new methods with a boolean parameter if call from the concurrent
>>> pool.
>>>
>>> It's a good way? or another better way exists?
>>
>> That is definitely better, but will it allow genuine duplicates to be
>> detected when using pooling?
>> I suspect it will, but have not tested this.
>>
>> Another possibility might be to allow setCookieManager etc. to allow a
>> null argument to reset the value without logging a warning.
>>
>> The pooling code could then use:
>>
>> setCookieManager(null);
>> setCookieManager(clonedCookieManager);
>>
>> Similarly for the other managers.
>>
>> However this is slightly less efficient as it updates the property twice.
>>
>> If we think there are other use cases for allowing the manager value
>> to be nullified then we would need to make the changes to the
>> setManager methods anyway.
>> At present, it looks like the code will generate an NPE when building
>> the log message.
>
> Another alternative: rather than use setCookieManager etc, the pooling
> code could set the property directly.
>
> e.g. add the method
>
> private void setCookieManagerProperty(CookieManager value) {
> setProperty(new TestElementProperty(COOKIE_MANAGER, value));
> }
>
> this can then be called by the original public method:
>
> public void setCookieManager(CookieManager value) {
> CookieManager mgr = getCookieManager();
> if (mgr != null) {
> log.warn("Existing CookieManager " + mgr.getName() + "
> superseded by " + value.getName());
> }
> setCookieManagerProperty(value);
> }
>
> [Similarly for setCacheManager]
>
> The pooling code is then changed to use setCookieManagerProperty
> rather than setCookieManager.
> This bypasses the checks without needing an extra boolean parameter.
I've tested this and it works.
The unnecessary warnings are no longer generated, but real warnings are logged.
So I have committed the code.
>>>
>>>
>>>>
>>>> Also, the code looks odd - why check for debug yet report a warning?
>>>>
>>>>> }
>>>>> setProperty(new TestElementProperty(AUTH_MANAGER, value));
>>>>> @@ -783,7 +783,7 @@ public abstract class HTTPSamplerBase ex
>>>>>
>>>>> public void setCookieManager(CookieManager value) {
>>>>> CookieManager mgr = getCookieManager();
>>>>> - if (mgr != null) {
>>>>> + if (log.isDebugEnabled()&& mgr != null) {
>>>>>
>>>>> log.warn("Existing CookieManager " + mgr.getName() + "
>>>>> superseded by " + value.getName());
>>>>> }
>>>>> setProperty(new TestElementProperty(COOKIE_MANAGER, value));
>>>>> @@ -795,7 +795,7 @@ public abstract class HTTPSamplerBase ex
>>>>>
>>>>> public void setCacheManager(CacheManager value) {
>>>>> CacheManager mgr = getCacheManager();
>>>>> - if (mgr != null) {
>>>>> + if (log.isDebugEnabled()&& mgr != null) {
>>>>>
>>>>> log.warn("Existing CacheManager " + mgr.getName() + "
>>>>> superseded by " + value.getName());
>>>>> }
>>>>> setProperty(new TestElementProperty(CACHE_MANAGER, value));
>>>>>
>>>>>
>>>
Re: svn commit: r1402574 - /jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http/sampler/HTTPSamplerBase.java
Posted by sebb <se...@gmail.com>.
On 1 November 2012 11:59, sebb <se...@gmail.com> wrote:
> On 29 October 2012 08:16, Milamber <mi...@apache.org> wrote:
>>
>>
>> Le 29/10/2012 00:54, sebb a ecrit :
>>>
>>> On 26 October 2012 18:04,<mi...@apache.org> wrote:
>>>>
>>>> Author: milamber
>>>> Date: Fri Oct 26 17:04:00 2012
>>>> New Revision: 1402574
>>>>
>>>> URL: http://svn.apache.org/viewvc?rev=1402574&view=rev
>>>> Log:
>>>> When used the options Retrieve All Embedded Resources + Concurrent pool,
>>>> these log.warn() generates a lot of lines in jmeter.log.
>>>>
>>>> Modified:
>>>>
>>>> jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http/sampler/HTTPSamplerBase.java
>>>
>>> -1, see below.
>>>
>>>> Modified:
>>>> jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http/sampler/HTTPSamplerBase.java
>>>> URL:
>>>> http://svn.apache.org/viewvc/jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http/sampler/HTTPSamplerBase.java?rev=1402574&r1=1402573&r2=1402574&view=diff
>>>>
>>>> ==============================================================================
>>>> ---
>>>> jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http/sampler/HTTPSamplerBase.java
>>>> (original)
>>>> +++
>>>> jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http/sampler/HTTPSamplerBase.java
>>>> Fri Oct 26 17:04:00 2012
>>>> @@ -753,7 +753,7 @@ public abstract class HTTPSamplerBase ex
>>>>
>>>> public void setAuthManager(AuthManager value) {
>>>> AuthManager mgr = getAuthManager();
>>>> - if (mgr != null) {
>>>> + if (log.isDebugEnabled()&& mgr != null) {
>>>>
>>>> log.warn("Existing AuthManager " + mgr.getName() + "
>>>> superseded by " + value.getName());
>>>
>>> I'm not very happy with this fix, as it suppresses the warnings when
>>> Concurrent pool is not in use.
>>> It also presumably does not display valid warnings if concurrent pool is
>>> in use.
>>>
>>> I've not looked at the code yet, but there has to be a better way to do
>>> this.
>>> Ideally change the pool code so it does not generate the overrides.
>>
>>
>> In attachment a patch to fix the first incorrect fix.
>> * revert on setAuthManager(), it's not necessary
>> * add 2 new methods with a boolean parameter if call from the concurrent
>> pool.
>>
>> It's a good way? or another better way exists?
>
> That is definitely better, but will it allow genuine duplicates to be
> detected when using pooling?
> I suspect it will, but have not tested this.
>
> Another possibility might be to allow setCookieManager etc. to allow a
> null argument to reset the value without logging a warning.
>
> The pooling code could then use:
>
> setCookieManager(null);
> setCookieManager(clonedCookieManager);
>
> Similarly for the other managers.
>
> However this is slightly less efficient as it updates the property twice.
>
> If we think there are other use cases for allowing the manager value
> to be nullified then we would need to make the changes to the
> setManager methods anyway.
> At present, it looks like the code will generate an NPE when building
> the log message.
Another alternative: rather than use setCookieManager etc, the pooling
code could set the property directly.
e.g. add the method
private void setCookieManagerProperty(CookieManager value) {
setProperty(new TestElementProperty(COOKIE_MANAGER, value));
}
this can then be called by the original public method:
public void setCookieManager(CookieManager value) {
CookieManager mgr = getCookieManager();
if (mgr != null) {
log.warn("Existing CookieManager " + mgr.getName() + "
superseded by " + value.getName());
}
setCookieManagerProperty(value);
}
[Similarly for setCacheManager]
The pooling code is then changed to use setCookieManagerProperty
rather than setCookieManager.
This bypasses the checks without needing an extra boolean parameter.
>>
>>
>>>
>>> Also, the code looks odd - why check for debug yet report a warning?
>>>
>>>> }
>>>> setProperty(new TestElementProperty(AUTH_MANAGER, value));
>>>> @@ -783,7 +783,7 @@ public abstract class HTTPSamplerBase ex
>>>>
>>>> public void setCookieManager(CookieManager value) {
>>>> CookieManager mgr = getCookieManager();
>>>> - if (mgr != null) {
>>>> + if (log.isDebugEnabled()&& mgr != null) {
>>>>
>>>> log.warn("Existing CookieManager " + mgr.getName() + "
>>>> superseded by " + value.getName());
>>>> }
>>>> setProperty(new TestElementProperty(COOKIE_MANAGER, value));
>>>> @@ -795,7 +795,7 @@ public abstract class HTTPSamplerBase ex
>>>>
>>>> public void setCacheManager(CacheManager value) {
>>>> CacheManager mgr = getCacheManager();
>>>> - if (mgr != null) {
>>>> + if (log.isDebugEnabled()&& mgr != null) {
>>>>
>>>> log.warn("Existing CacheManager " + mgr.getName() + "
>>>> superseded by " + value.getName());
>>>> }
>>>> setProperty(new TestElementProperty(CACHE_MANAGER, value));
>>>>
>>>>
>>
Re: svn commit: r1402574 - /jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http/sampler/HTTPSamplerBase.java
Posted by sebb <se...@gmail.com>.
On 29 October 2012 08:16, Milamber <mi...@apache.org> wrote:
>
>
> Le 29/10/2012 00:54, sebb a ecrit :
>>
>> On 26 October 2012 18:04,<mi...@apache.org> wrote:
>>>
>>> Author: milamber
>>> Date: Fri Oct 26 17:04:00 2012
>>> New Revision: 1402574
>>>
>>> URL: http://svn.apache.org/viewvc?rev=1402574&view=rev
>>> Log:
>>> When used the options Retrieve All Embedded Resources + Concurrent pool,
>>> these log.warn() generates a lot of lines in jmeter.log.
>>>
>>> Modified:
>>>
>>> jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http/sampler/HTTPSamplerBase.java
>>
>> -1, see below.
>>
>>> Modified:
>>> jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http/sampler/HTTPSamplerBase.java
>>> URL:
>>> http://svn.apache.org/viewvc/jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http/sampler/HTTPSamplerBase.java?rev=1402574&r1=1402573&r2=1402574&view=diff
>>>
>>> ==============================================================================
>>> ---
>>> jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http/sampler/HTTPSamplerBase.java
>>> (original)
>>> +++
>>> jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http/sampler/HTTPSamplerBase.java
>>> Fri Oct 26 17:04:00 2012
>>> @@ -753,7 +753,7 @@ public abstract class HTTPSamplerBase ex
>>>
>>> public void setAuthManager(AuthManager value) {
>>> AuthManager mgr = getAuthManager();
>>> - if (mgr != null) {
>>> + if (log.isDebugEnabled()&& mgr != null) {
>>>
>>> log.warn("Existing AuthManager " + mgr.getName() + "
>>> superseded by " + value.getName());
>>
>> I'm not very happy with this fix, as it suppresses the warnings when
>> Concurrent pool is not in use.
>> It also presumably does not display valid warnings if concurrent pool is
>> in use.
>>
>> I've not looked at the code yet, but there has to be a better way to do
>> this.
>> Ideally change the pool code so it does not generate the overrides.
>
>
> In attachment a patch to fix the first incorrect fix.
> * revert on setAuthManager(), it's not necessary
> * add 2 new methods with a boolean parameter if call from the concurrent
> pool.
>
> It's a good way? or another better way exists?
That is definitely better, but will it allow genuine duplicates to be
detected when using pooling?
I suspect it will, but have not tested this.
Another possibility might be to allow setCookieManager etc. to allow a
null argument to reset the value without logging a warning.
The pooling code could then use:
setCookieManager(null);
setCookieManager(clonedCookieManager);
Similarly for the other managers.
However this is slightly less efficient as it updates the property twice.
If we think there are other use cases for allowing the manager value
to be nullified then we would need to make the changes to the
setManager methods anyway.
At present, it looks like the code will generate an NPE when building
the log message.
>
>
>>
>> Also, the code looks odd - why check for debug yet report a warning?
>>
>>> }
>>> setProperty(new TestElementProperty(AUTH_MANAGER, value));
>>> @@ -783,7 +783,7 @@ public abstract class HTTPSamplerBase ex
>>>
>>> public void setCookieManager(CookieManager value) {
>>> CookieManager mgr = getCookieManager();
>>> - if (mgr != null) {
>>> + if (log.isDebugEnabled()&& mgr != null) {
>>>
>>> log.warn("Existing CookieManager " + mgr.getName() + "
>>> superseded by " + value.getName());
>>> }
>>> setProperty(new TestElementProperty(COOKIE_MANAGER, value));
>>> @@ -795,7 +795,7 @@ public abstract class HTTPSamplerBase ex
>>>
>>> public void setCacheManager(CacheManager value) {
>>> CacheManager mgr = getCacheManager();
>>> - if (mgr != null) {
>>> + if (log.isDebugEnabled()&& mgr != null) {
>>>
>>> log.warn("Existing CacheManager " + mgr.getName() + "
>>> superseded by " + value.getName());
>>> }
>>> setProperty(new TestElementProperty(CACHE_MANAGER, value));
>>>
>>>
>
Re: svn commit: r1402574 - /jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http/sampler/HTTPSamplerBase.java
Posted by Milamber <mi...@apache.org>.
Le 29/10/2012 00:54, sebb a ecrit :
> On 26 October 2012 18:04,<mi...@apache.org> wrote:
>> Author: milamber
>> Date: Fri Oct 26 17:04:00 2012
>> New Revision: 1402574
>>
>> URL: http://svn.apache.org/viewvc?rev=1402574&view=rev
>> Log:
>> When used the options Retrieve All Embedded Resources + Concurrent pool, these log.warn() generates a lot of lines in jmeter.log.
>>
>> Modified:
>> jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http/sampler/HTTPSamplerBase.java
> -1, see below.
>
>> Modified: jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http/sampler/HTTPSamplerBase.java
>> URL: http://svn.apache.org/viewvc/jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http/sampler/HTTPSamplerBase.java?rev=1402574&r1=1402573&r2=1402574&view=diff
>> ==============================================================================
>> --- jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http/sampler/HTTPSamplerBase.java (original)
>> +++ jmeter/trunk/src/protocol/http/org/apache/jmeter/protocol/http/sampler/HTTPSamplerBase.java Fri Oct 26 17:04:00 2012
>> @@ -753,7 +753,7 @@ public abstract class HTTPSamplerBase ex
>>
>> public void setAuthManager(AuthManager value) {
>> AuthManager mgr = getAuthManager();
>> - if (mgr != null) {
>> + if (log.isDebugEnabled()&& mgr != null) {
>> log.warn("Existing AuthManager " + mgr.getName() + " superseded by " + value.getName());
> I'm not very happy with this fix, as it suppresses the warnings when
> Concurrent pool is not in use.
> It also presumably does not display valid warnings if concurrent pool is in use.
>
> I've not looked at the code yet, but there has to be a better way to do this.
> Ideally change the pool code so it does not generate the overrides.
In attachment a patch to fix the first incorrect fix.
* revert on setAuthManager(), it's not necessary
* add 2 new methods with a boolean parameter if call from the concurrent
pool.
It's a good way? or another better way exists?
>
> Also, the code looks odd - why check for debug yet report a warning?
>
>> }
>> setProperty(new TestElementProperty(AUTH_MANAGER, value));
>> @@ -783,7 +783,7 @@ public abstract class HTTPSamplerBase ex
>>
>> public void setCookieManager(CookieManager value) {
>> CookieManager mgr = getCookieManager();
>> - if (mgr != null) {
>> + if (log.isDebugEnabled()&& mgr != null) {
>> log.warn("Existing CookieManager " + mgr.getName() + " superseded by " + value.getName());
>> }
>> setProperty(new TestElementProperty(COOKIE_MANAGER, value));
>> @@ -795,7 +795,7 @@ public abstract class HTTPSamplerBase ex
>>
>> public void setCacheManager(CacheManager value) {
>> CacheManager mgr = getCacheManager();
>> - if (mgr != null) {
>> + if (log.isDebugEnabled()&& mgr != null) {
>> log.warn("Existing CacheManager " + mgr.getName() + " superseded by " + value.getName());
>> }
>> setProperty(new TestElementProperty(CACHE_MANAGER, value));
>>
>>