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

Support variable for all JMS messages (bytes, object, ...) and sources (file, folder) #241

Closed
wants to merge 6 commits into
base: trunk
from

Conversation

Projects
None yet
4 participants
@max3163
Contributor

max3163 commented Dec 22, 2016

Source code is based on Java 8 and unit tests has been added. Message format processing has been moved to a dedicated package, same for cache support.

An encoding field has been added to UI. It is by default filled with two special values (<RAW>, <DEFAULT> and standard charsets).

<RAW> acts exactly as current version. No variable support and load files with default system charset. <DEFAULT> mode applies default system one, except for XML which relies on XML prolog. In other cases, the specified encoding is use whatever it is valid or not.

Cache system use a single field in PublisherSampler, so explicit type of cached content is not known. It may be improved (replaced ?) by using Guava instead. Cached content isn't consistent for a message type (bytes, object, ...) but also depends if raw mode is used or not. For all non-raw mode, text is always cached to permit variable processing for each sampler execution.

Two new components were added to JMeter test API:

  • ResourceLocator: enables to locate a resource file and give a NIO Path or String one. Relying on pure URL causes problem with path containing some special characters such as space (%20). It solves an existing problem on current code base.
  • JMeterContextServiceHelper: it consits in a JUnit rule implementation to manage thread-bound singletons of JMeterContext. See TextMessageRendererTest for an example usage.

Special notes: in order to check encoding management, test resources encoding must be preserved. By default, all files are in UTF-8 ; except for: cp1252.txt, object_cp1252.txt, object_prolog_cp1252.xml,

@FSchumacher

This comment has been minimized.

Show comment
Hide comment
@FSchumacher

FSchumacher Dec 27, 2016

Contributor

Thanks for your contribution. Could you resubmit your PR based on current trunk? It is really hard to see your changes otherwise.

On the addition of a cache: we have added caffeine. You might want to have a look at it for your caching needs.

Contributor

FSchumacher commented Dec 27, 2016

Thanks for your contribution. Could you resubmit your PR based on current trunk? It is really hard to see your changes otherwise.

On the addition of a cache: we have added caffeine. You might want to have a look at it for your caching needs.

@loganmzz

This comment has been minimized.

Show comment
Hide comment
@loganmzz

loganmzz Dec 27, 2016

Yes I can give it a try. However, can you suggest a stable commit ? Or I can just use anyone ?

PS : I'm co-author of the PR.

loganmzz commented Dec 27, 2016

Yes I can give it a try. However, can you suggest a stable commit ? Or I can just use anyone ?

PS : I'm co-author of the PR.

@FSchumacher

This comment has been minimized.

Show comment
Hide comment
@FSchumacher

FSchumacher Dec 27, 2016

Contributor

Well, try a current one. It would be great if there is no conflict.

Contributor

FSchumacher commented Dec 27, 2016

Well, try a current one. It would be great if there is no conflict.

@pmouawad

This comment has been minimized.

Show comment
Hide comment
@pmouawad

pmouawad Dec 28, 2016

Contributor

Hello @loganmzz ,
Is this PR a duplicate of PR 239 ?
If so, could you close 239?
Did you check the performance impact of this feature ?

Thanks a lot for your contribution.
Regards

Contributor

pmouawad commented Dec 28, 2016

Hello @loganmzz ,
Is this PR a duplicate of PR 239 ?
If so, could you close 239?
Did you check the performance impact of this feature ?

Thanks a lot for your contribution.
Regards

@loganmzz

This comment has been minimized.

Show comment
Hide comment
@loganmzz

loganmzz Dec 29, 2016

Yes, it finally replaces #239. I have first supposed both will be merged, but finally rewritten to improve clean code with separation of concerns.

Regarding performance, "raw" mode may have few overhead. Variable support should act exactly as if text area was used. I will try to make some check with microbenchmarking.

loganmzz commented Dec 29, 2016

Yes, it finally replaces #239. I have first supposed both will be merged, but finally rewritten to improve clean code with separation of concerns.

Regarding performance, "raw" mode may have few overhead. Variable support should act exactly as if text area was used. I will try to make some check with microbenchmarking.

@loganmzz

This comment has been minimized.

Show comment
Hide comment
@loganmzz

loganmzz Jan 5, 2017

I have rebase my works on trunk: d29ac21 (Sonar : fix squid:UselessParenthesesCheck Remove those useless parentheses) and replaces the simple cache API by Caffeine

loganmzz commented Jan 5, 2017

I have rebase my works on trunk: d29ac21 (Sonar : fix squid:UselessParenthesesCheck Remove those useless parentheses) and replaces the simple cache API by Caffeine

@pmouawad

This comment has been minimized.

Show comment
Hide comment
@pmouawad

pmouawad Jan 15, 2017

Contributor

Hello,
Before merging this one , I'd like to be sure it does not introduce performance degradation.

Did you have time to make a micro benchmark ?
Thank you

Contributor

pmouawad commented Jan 15, 2017

Hello,
Before merging this one , I'd like to be sure it does not introduce performance degradation.

Did you have time to make a micro benchmark ?
Thank you

@max3163

This comment has been minimized.

Show comment
Hide comment
@max3163

max3163 Jan 16, 2017

Contributor

Protocol :

Send 200000 messages share on 2 threads to a JMS topics (text format on a local ActiveMQ broker )
Send 200000 messages share on 2 threads to a JMS topics (binary format on a local ActiveMQ broker )

  1. JMeter 3.1 :
  • Throughput :
    BINARY : 11 400 tps
    TEXT : 11 600 tps
  1. JMeter 3.2 with PR #241
  • Encodage ROW
    Throughput :
    BINARY : 12 700 tps
    TEXT : 11 500 tps

  • Encodage UTF-8 ( no variable in JMETER )
    Throughput :
    BINARY : 11 700 tps
    TEXT : 9 650 tps

  • Encodage UTF-8 ( with variable set in JMeter )
    Throughput :
    BINARY : 12 000 tps
    TEXT : 9 700 tps

As you can see, other than one impact on the binary side when you activate the Encodage – and it’s normal as the file is interpreted - the results are almost identical.

Contributor

max3163 commented Jan 16, 2017

Protocol :

Send 200000 messages share on 2 threads to a JMS topics (text format on a local ActiveMQ broker )
Send 200000 messages share on 2 threads to a JMS topics (binary format on a local ActiveMQ broker )

  1. JMeter 3.1 :
  • Throughput :
    BINARY : 11 400 tps
    TEXT : 11 600 tps
  1. JMeter 3.2 with PR #241
  • Encodage ROW
    Throughput :
    BINARY : 12 700 tps
    TEXT : 11 500 tps

  • Encodage UTF-8 ( no variable in JMETER )
    Throughput :
    BINARY : 11 700 tps
    TEXT : 9 650 tps

  • Encodage UTF-8 ( with variable set in JMeter )
    Throughput :
    BINARY : 12 000 tps
    TEXT : 9 700 tps

As you can see, other than one impact on the binary side when you activate the Encodage – and it’s normal as the file is interpreted - the results are almost identical.

@max3163

This comment has been minimized.

Show comment
Hide comment
@max3163

max3163 Jan 26, 2017

Contributor

Did you have the opportunity to review theses commits ?
It's important change we want to share with you to use in production the stock version of JMeter rather than our own build.

Contributor

max3163 commented Jan 26, 2017

Did you have the opportunity to review theses commits ?
It's important change we want to share with you to use in production the stock version of JMeter rather than our own build.

@pmouawad

This comment has been minimized.

Show comment
Hide comment
@pmouawad

pmouawad Feb 16, 2017

Contributor

Hello,
Would it be possible to bench this with more threads than 2 ?
50 for example ?
Would it be possible to wait for 3.2 release before merging this one ?
Thanks

Contributor

pmouawad commented Feb 16, 2017

Hello,
Would it be possible to bench this with more threads than 2 ?
50 for example ?
Would it be possible to wait for 3.2 release before merging this one ?
Thanks

@max3163 max3163 closed this Feb 16, 2017

@max3163

This comment has been minimized.

Show comment
Hide comment
@max3163

max3163 Feb 17, 2017

Contributor

Closed with the 1783319 commit

Contributor

max3163 commented Feb 17, 2017

Closed with the 1783319 commit

@max3163

This comment has been minimized.

Show comment
Hide comment
@max3163

max3163 Feb 17, 2017

Contributor

@pmouawad

I use this patch in production for 3 month with big load and big data without any problem of performance.
This patch add a new behaviour only for file part, but the behaviour is the same than before for data put in the textfield.
If you put RAW encoding, JMeter don't try to parse the file in order to remplace variable, so you turned into the default old behaviour.
For me, there is no objective risk to apply this patch.

Sorry if I did it to quickly....

Contributor

max3163 commented Feb 17, 2017

@pmouawad

I use this patch in production for 3 month with big load and big data without any problem of performance.
This patch add a new behaviour only for file part, but the behaviour is the same than before for data put in the textfield.
If you put RAW encoding, JMeter don't try to parse the file in order to remplace variable, so you turned into the default old behaviour.
For me, there is no objective risk to apply this patch.

Sorry if I did it to quickly....

@max3163 max3163 deleted the max3163:POW-322 branch Apr 27, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment