[GitHub] activemq-artemis pull request #2034: ARTEMIS-1818 re-create auto-created que...

[jbertram]

Github user jbertram commented on a diff in the pull request:

https://github.com/apache/activemq-artemis/pull/2034#discussion_r182924057
  
--- Diff: 
artemis-core-client/src/main/java/org/apache/activemq/artemis/core/protocol/core/impl/ActiveMQSessionContext.java
 ---
@@ -772,6 +772,10 @@ public void 
recreateConsumerOnServer(ClientConsumerInternal consumerInternal,
   if (!queueInfo.isDurable()) {
  CreateQueueMessage_V2 createQueueRequest = new 
CreateQueueMessage_V2(queueInfo.getAddress(), queueInfo.getName(), 
queueInfo.getRoutingType(), queueInfo.getFilterString(), false, 
queueInfo.isTemporary(), queueInfo.getMaxConsumers(), 
queueInfo.isPurgeOnNoConsumers(), queueInfo.isAutoCreated(), false, 
queueInfo.isExclusive(), queueInfo.isLastValue());
 
+ sendPacketWithoutLock(sessionChannel, createQueueRequest);
+  } else if (queueInfo.isAutoCreated()) {
--- End diff --

Will do, @michaelandrepearce. That's a much more elegant solution.


---

[GitHub] activemq-artemis pull request #2034: ARTEMIS-1818 re-create auto-created que...

[michaelandrepearce]

Github user michaelandrepearce commented on a diff in the pull request:

https://github.com/apache/activemq-artemis/pull/2034#discussion_r182906602
  
--- Diff: 
artemis-core-client/src/main/java/org/apache/activemq/artemis/core/protocol/core/impl/ActiveMQSessionContext.java
 ---
@@ -772,6 +772,10 @@ public void 
recreateConsumerOnServer(ClientConsumerInternal consumerInternal,
   if (!queueInfo.isDurable()) {
  CreateQueueMessage_V2 createQueueRequest = new 
CreateQueueMessage_V2(queueInfo.getAddress(), queueInfo.getName(), 
queueInfo.getRoutingType(), queueInfo.getFilterString(), false, 
queueInfo.isTemporary(), queueInfo.getMaxConsumers(), 
queueInfo.isPurgeOnNoConsumers(), queueInfo.isAutoCreated(), false, 
queueInfo.isExclusive(), queueInfo.isLastValue());
 
+ sendPacketWithoutLock(sessionChannel, createQueueRequest);
+  } else if (queueInfo.isAutoCreated()) {
--- End diff --

seems almost duplicated code lines.

why not make the if statement of the if above (!queueInfo.isDurable() || 
queueInfo.isAutoCreated()) 
and replace the only difference which is the hardcoded false for durability 
to take queueInfo.isDurable().


---

[GitHub] activemq-artemis issue #2015: ARTEMIS-1807 File-based Large Message encoding...

[franz1981]

Github user franz1981 commented on the issue:

https://github.com/apache/activemq-artemis/pull/2015
  
@michaelandrepearce @mtaylor @clebertsuconic @gaohoward Guys I was looking 
to the improvements made by this PR andI found that there is a log of 
Journal activity with it! I meanASYNCIO ones...it makes sense? 
That activity seems the bottleneck on consumer side


---

Re: [DISCUSS] Separate commit for Test and Fix on PRs

[Clebert Suconic]

will I sound crazy if I change my mind here?

I thought it would be improving the PR process.. but on a second
thought... it won't improve things actually and i will agree with
Robbie here.

 So. .just ignore me.. and apologize for spamming  you guys..

On Thu, Apr 19, 2018 at 10:48 AM, Timothy Bish  wrote:
> On 04/19/2018 10:32 AM, Robbie Gemmell wrote:
>>
>> I dont think its unreasonable or unexpected in many cases that a test
>> fails to compile without the changes in relates to. It depends what
>> type of test it is and what the changes actually are though.
>>
>> I agree it wont hugely change the PR though. Possibly why I prefer
>> them being in the same commit is I tend to use the commit to look over
>> things rather than the PR.
>
>
> The other thing to keep in mind is that when two or more commits for the
> same bit of work go in, the process of reverting changes becomes more error
> prone as the person doing the reverts has to always be looking for the cases
> where there was more than one related to the same scope of work.
>
>
>> On 19 April 2018 at 15:10, Clebert Suconic 
>> wrote:
>>>
>>> I have seen a few cases where the test would not compile.. that is the
>>> test depends on the changes itself. What is not really Test Driven
>>> Development.
>>>
>>> Both tests and fixes are part of the same PR.. so it doesn't really
>>> change much.
>>>
>>> On Thu, Apr 19, 2018 at 9:41 AM, Robbie Gemmell
>>>  wrote:

 In general I think having tests and changes in the same commit is
 nicer, especially for looking back at later.

 I'll also often apply a test on its own or revert the non-test changes
 to ensure tests fail, I've not really found it slow/annoying enough to
 specifically seperate tests out in their own commits to facilitate it.

 Robbie

 On 18 April 2018 at 18:27, Clebert Suconic 
 wrote:
>
> I would appreciate if we separated fixes and tests on Pull Requests.
>
>
> A lot of times i will revert the fixes to validate if the test is good
> (if it fails without a fix) and how it failed. (not that I don't trust
> the committer, just part of the validation as sometimes I want to see
> what was the semantic change and fix). I may eventually play a better
> fix in the process.. and I am sure that would apply to anyone else
> helping on reviewing commits.
>
>
> I had at some point gone back in history and needed to apply the test
> without a fix to find a better fix.
>
> I know eventually it's not possible to separate these.. but if you
> could as much as possible separate them:?
>
>
> I recently did that into PR #2004...
>
>
> https://github.com/apache/activemq-artemis/commit/a72046a0e32fd47cad988a8d71512927f74c8585
>
>
> https://github.com/apache/activemq-artemis/commit/a72046a0e32fd47cad988a8d71512927f74c8585
>
>
>
>
> I may update the hacking guide with this.. WDYT?
>
>
> --
> Clebert Suconic
>>>
>>>
>>>
>>> --
>>> Clebert Suconic
>
>
>
> --
> Tim Bish
> twitter: @tabish121
> blog: http://timbish.blogspot.com/
>



-- 
Clebert Suconic

[GitHub] activemq-artemis issue #2031: ARTEMIS-1816 OpenWire should avoid ByteArrayOu...

[clebertsuconic]

Github user clebertsuconic commented on the issue:

https://github.com/apache/activemq-artemis/pull/2031
  
@tabish121 @franz1981 fair enough.. lets run the whole testsuite then.. 
then we can merge this.


---

[GitHub] activemq-artemis pull request #2034: ARTEMIS-1818 re-create auto-created que...

[jbertram]

GitHub user jbertram opened a pull request:

https://github.com/apache/activemq-artemis/pull/2034

ARTEMIS-1818 re-create auto-created queue on JMS reconnect



You can merge this pull request into a Git repository by running:

$ git pull https://github.com/jbertram/activemq-artemis ARTEMIS-1818

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/activemq-artemis/pull/2034.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #2034


commit 94fc317081f3ac3a6f87d21e8f076be08e660aa7
Author: Justin Bertram 
Date:   2018-04-19T19:42:39Z

ARTEMIS-1818 re-create auto-created queue on JMS reconnect




---

[GitHub] activemq-artemis issue #2031: ARTEMIS-1816 OpenWire should avoid ByteArrayOu...

[tabish121]

Github user tabish121 commented on the issue:

https://github.com/apache/activemq-artemis/pull/2031
  
The change as it is seems acceptable as it does reduce unnecessary 
allocations when ByteMessages types cross to OpenWire.  I've reviewed the 
changes as it relates to the OpenWire objects and the way the converter uses it 
and I don't see anything adverse happening from this.  As always a run through 
the tests would be good for some validation.  


---

[GitHub] activemq-artemis issue #2031: ARTEMIS-1816 OpenWire should avoid ByteArrayOu...

[franz1981]

Github user franz1981 commented on the issue:

https://github.com/apache/activemq-artemis/pull/2031
  
I agree but given the short window until a new release i have preferred to 
push this (that's quite stable and safe) and taking my time to improve the 
other optimal way. That pr currently broke (last time I have checked) too many 
tests and I think it worth to make it stable without rushing.



---

[GitHub] activemq-artemis issue #2031: ARTEMIS-1816 OpenWire should avoid ByteArrayOu...

[clebertsuconic]

Github user clebertsuconic commented on the issue:

https://github.com/apache/activemq-artemis/pull/2031
  
@franz1981 sorry.. but I -1 on this.. you ok with closing this?


---

[GitHub] activemq-artemis pull request #2033: ARTEMIS-1793 fix property detection

[asfgit]

Github user asfgit closed the pull request at:

https://github.com/apache/activemq-artemis/pull/2033


---

[GitHub] activemq-artemis issue #2031: ARTEMIS-1816 OpenWire should avoid ByteArrayOu...

[tabish121]

Github user tabish121 commented on the issue:

https://github.com/apache/activemq-artemis/pull/2031
  
That would be the more ideal thing to do yes. 


---

[GitHub] activemq-artemis issue #2031: ARTEMIS-1816 OpenWire should avoid ByteArrayOu...

[clebertsuconic]

Github user clebertsuconic commented on the issue:

https://github.com/apache/activemq-artemis/pull/2031
  
for optimizing OpenWire.. I would prefer implementing OpenWireMessage 
properly. There's a PR open for that already.


---

[GitHub] activemq-artemis issue #2031: ARTEMIS-1816 OpenWire should avoid ByteArrayOu...

[tabish121]

Github user tabish121 commented on the issue:

https://github.com/apache/activemq-artemis/pull/2031
  
@franz1981 So I think that you could avoid subclassing the 
ActiveMQBytesMessage if you built the properties yourself instead of calling 
setObjectProperty in setAMQMsgObjectProperties in the converter as that is 
where the trigger point is for allocating the bytes stream (see 
initializeWriting and its uses in ActiveMQBytesMessage).  In a manner similar 
to how the converter puts the body into the message using setContent you could 
create and marshal the properties and store them using setMarshalledProperties 
in the OpenWire base Message class.  The properties are marshalled using the 
utility class MarshallingSupport#marshalPrimitiveMap so it is possible to roll 
your own and then bypass that bit of the ActiveMQMessage's normal handling.  


---

[GitHub] activemq-artemis pull request #2033: ARTEMIS-1793 fix property detection

[jbertram]

GitHub user jbertram opened a pull request:

https://github.com/apache/activemq-artemis/pull/2033

ARTEMIS-1793 fix property detection



You can merge this pull request into a Git repository by running:

$ git pull https://github.com/jbertram/activemq-artemis ARTEMIS-1793

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/activemq-artemis/pull/2033.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #2033


commit 48e066b0c1866b7739d355a6a9b1ef14a786ec03
Author: Justin Bertram 
Date:   2018-04-19T16:25:03Z

ARTEMIS-1793 fix property detection




---

[GitHub] activemq-artemis issue #2031: ARTEMIS-1816 OpenWire should avoid ByteArrayOu...

[franz1981]

Github user franz1981 commented on the issue:

https://github.com/apache/activemq-artemis/pull/2031
  
@tabish121 I'm sorry to have "hacked" like this the Open Wire class. The 
best solution would be to avoid at all any allocation of 
`ActiveMQBytesMessage`, any idea?


---

[GitHub] activemq-artemis pull request #2032: ARTEMIS-1817 fix getting non-existent p...

[jbertram]

Github user jbertram closed the pull request at:

https://github.com/apache/activemq-artemis/pull/2032


---

[GitHub] activemq-artemis pull request #2032: ARTEMIS-1817 fix getting non-existent p...

[jbertram]

GitHub user jbertram opened a pull request:

https://github.com/apache/activemq-artemis/pull/2032

ARTEMIS-1817 fix getting non-existent properties

Calling valueOf() on certain types with 'null' will actuall throw an
exception (e.g. NumberFormat exception for java.lang.Double) rather than
just returning null.

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/jbertram/activemq-artemis ARTEMIS-1817

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/activemq-artemis/pull/2032.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #2032


commit fb2c423595c2000a8bc9326c33963f7f7ccbb892
Author: Justin Bertram 
Date:   2018-04-19T16:14:04Z

ARTEMIS-1817 fix getting non-existent properties

Calling valueOf() on certain types with 'null' will actuall throw an
exception (e.g. NumberFormat exception for java.lang.Double) rather than
just returning null.




---

[GitHub] activemq-artemis issue #2031: ARTEMIS-1816 OpenWire should avoid ByteArrayOu...

[franz1981]

Github user franz1981 commented on the issue:

https://github.com/apache/activemq-artemis/pull/2031
  
@clebertsuconic I need to run all the CI tests on it to be safe :+1: 


---

[GitHub] activemq-artemis pull request #2031: ARTEMIS-1816 OpenWire should avoid Byte...

[franz1981]

GitHub user franz1981 opened a pull request:

https://github.com/apache/activemq-artemis/pull/2031

ARTEMIS-1816 OpenWire should avoid ByteArrayOutputStream lazy allocation

OpenWireMessageConverter::toAMQMessage on bytes messages is lazy
allocating a write buffer with a default size of 1024 even when
it won't be used to write anything.
It avoid an useless allocation by reducing it to new byte[0].

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/franz1981/activemq-artemis ARTEMIS-1816

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/activemq-artemis/pull/2031.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #2031


commit a391704c54d1d73e0dcc57e5c71a384e6299e3cd
Author: Francesco Nigro 
Date:   2018-04-19T15:46:26Z

ARTEMIS-1816 OpenWire should avoid ByteArrayOutputStream lazy allocation

OpenWireMessageConverter::toAMQMessage on bytes messages is lazy
allocating a write buffer with a default size of 1024 even when
it won't be used to write anything.
It avoid an useless allocation by reducing it to new byte[0].




---

[GitHub] activemq-artemis pull request #2030: Adds the Travis CI build status label o...

[asfgit]

Github user asfgit closed the pull request at:

https://github.com/apache/activemq-artemis/pull/2030


---

[GitHub] activemq-artemis issue #2030: Adds the Travis CI build status label on the R...

[orpiske]

Github user orpiske commented on the issue:

https://github.com/apache/activemq-artemis/pull/2030
  
One additional suggestion I received was to, possibly, put a label for all 
the active branches. 

Thoughts?


---

[GitHub] activemq-artemis pull request #2030: Adds the Travis CI build status label o...

[orpiske]

GitHub user orpiske opened a pull request:

https://github.com/apache/activemq-artemis/pull/2030

Adds the Travis CI build status label on the README

Some of my colleagues suggested it would be useful to have the Travis CI 
build label right when accessing the project page.

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/orpiske/activemq-artemis build-label

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/activemq-artemis/pull/2030.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #2030


commit af061f7db467657c46b776ef9a057f494a59ae94
Author: Otavio Rodolfo Piske 
Date:   2018-04-19T14:45:58Z

Adds the Travis CI build status label on the README




---

Re: [DISCUSS] Separate commit for Test and Fix on PRs

[Timothy Bish]

On 04/19/2018 10:32 AM, Robbie Gemmell wrote:

I dont think its unreasonable or unexpected in many cases that a test
fails to compile without the changes in relates to. It depends what
type of test it is and what the changes actually are though.

I agree it wont hugely change the PR though. Possibly why I prefer
them being in the same commit is I tend to use the commit to look over
things rather than the PR.


The other thing to keep in mind is that when two or more commits for the 
same bit of work go in, the process of reverting changes becomes more 
error prone as the person doing the reverts has to always be looking for 
the cases where there was more than one related to the same scope of work.



On 19 April 2018 at 15:10, Clebert Suconic  wrote:

I have seen a few cases where the test would not compile.. that is the
test depends on the changes itself. What is not really Test Driven
Development.

Both tests and fixes are part of the same PR.. so it doesn't really change much.

On Thu, Apr 19, 2018 at 9:41 AM, Robbie Gemmell
 wrote:

In general I think having tests and changes in the same commit is
nicer, especially for looking back at later.

I'll also often apply a test on its own or revert the non-test changes
to ensure tests fail, I've not really found it slow/annoying enough to
specifically seperate tests out in their own commits to facilitate it.

Robbie

On 18 April 2018 at 18:27, Clebert Suconic  wrote:

I would appreciate if we separated fixes and tests on Pull Requests.


A lot of times i will revert the fixes to validate if the test is good
(if it fails without a fix) and how it failed. (not that I don't trust
the committer, just part of the validation as sometimes I want to see
what was the semantic change and fix). I may eventually play a better
fix in the process.. and I am sure that would apply to anyone else
helping on reviewing commits.


I had at some point gone back in history and needed to apply the test
without a fix to find a better fix.

I know eventually it's not possible to separate these.. but if you
could as much as possible separate them:?


I recently did that into PR #2004...

https://github.com/apache/activemq-artemis/commit/a72046a0e32fd47cad988a8d71512927f74c8585

https://github.com/apache/activemq-artemis/commit/a72046a0e32fd47cad988a8d71512927f74c8585




I may update the hacking guide with this.. WDYT?


--
Clebert Suconic



--
Clebert Suconic



--
Tim Bish
twitter: @tabish121
blog: http://timbish.blogspot.com/

Re: [DISCUSS] Separate commit for Test and Fix on PRs

[Robbie Gemmell]

I dont think its unreasonable or unexpected in many cases that a test
fails to compile without the changes in relates to. It depends what
type of test it is and what the changes actually are though.

I agree it wont hugely change the PR though. Possibly why I prefer
them being in the same commit is I tend to use the commit to look over
things rather than the PR.

On 19 April 2018 at 15:10, Clebert Suconic  wrote:
> I have seen a few cases where the test would not compile.. that is the
> test depends on the changes itself. What is not really Test Driven
> Development.
>
> Both tests and fixes are part of the same PR.. so it doesn't really change 
> much.
>
> On Thu, Apr 19, 2018 at 9:41 AM, Robbie Gemmell
>  wrote:
>> In general I think having tests and changes in the same commit is
>> nicer, especially for looking back at later.
>>
>> I'll also often apply a test on its own or revert the non-test changes
>> to ensure tests fail, I've not really found it slow/annoying enough to
>> specifically seperate tests out in their own commits to facilitate it.
>>
>> Robbie
>>
>> On 18 April 2018 at 18:27, Clebert Suconic  wrote:
>>> I would appreciate if we separated fixes and tests on Pull Requests.
>>>
>>>
>>> A lot of times i will revert the fixes to validate if the test is good
>>> (if it fails without a fix) and how it failed. (not that I don't trust
>>> the committer, just part of the validation as sometimes I want to see
>>> what was the semantic change and fix). I may eventually play a better
>>> fix in the process.. and I am sure that would apply to anyone else
>>> helping on reviewing commits.
>>>
>>>
>>> I had at some point gone back in history and needed to apply the test
>>> without a fix to find a better fix.
>>>
>>> I know eventually it's not possible to separate these.. but if you
>>> could as much as possible separate them:?
>>>
>>>
>>> I recently did that into PR #2004...
>>>
>>> https://github.com/apache/activemq-artemis/commit/a72046a0e32fd47cad988a8d71512927f74c8585
>>>
>>> https://github.com/apache/activemq-artemis/commit/a72046a0e32fd47cad988a8d71512927f74c8585
>>>
>>>
>>>
>>>
>>> I may update the hacking guide with this.. WDYT?
>>>
>>>
>>> --
>>> Clebert Suconic
>
>
>
> --
> Clebert Suconic

Re: [DISCUSS] Separate commit for Test and Fix on PRs

[Clebert Suconic]

I have seen a few cases where the test would not compile.. that is the
test depends on the changes itself. What is not really Test Driven
Development.

Both tests and fixes are part of the same PR.. so it doesn't really change much.

On Thu, Apr 19, 2018 at 9:41 AM, Robbie Gemmell
 wrote:
> In general I think having tests and changes in the same commit is
> nicer, especially for looking back at later.
>
> I'll also often apply a test on its own or revert the non-test changes
> to ensure tests fail, I've not really found it slow/annoying enough to
> specifically seperate tests out in their own commits to facilitate it.
>
> Robbie
>
> On 18 April 2018 at 18:27, Clebert Suconic  wrote:
>> I would appreciate if we separated fixes and tests on Pull Requests.
>>
>>
>> A lot of times i will revert the fixes to validate if the test is good
>> (if it fails without a fix) and how it failed. (not that I don't trust
>> the committer, just part of the validation as sometimes I want to see
>> what was the semantic change and fix). I may eventually play a better
>> fix in the process.. and I am sure that would apply to anyone else
>> helping on reviewing commits.
>>
>>
>> I had at some point gone back in history and needed to apply the test
>> without a fix to find a better fix.
>>
>> I know eventually it's not possible to separate these.. but if you
>> could as much as possible separate them:?
>>
>>
>> I recently did that into PR #2004...
>>
>> https://github.com/apache/activemq-artemis/commit/a72046a0e32fd47cad988a8d71512927f74c8585
>>
>> https://github.com/apache/activemq-artemis/commit/a72046a0e32fd47cad988a8d71512927f74c8585
>>
>>
>>
>>
>> I may update the hacking guide with this.. WDYT?
>>
>>
>> --
>> Clebert Suconic



-- 
Clebert Suconic

Re: [DISCUSS] Separate commit for Test and Fix on PRs

[Robbie Gemmell]

In general I think having tests and changes in the same commit is
nicer, especially for looking back at later.

I'll also often apply a test on its own or revert the non-test changes
to ensure tests fail, I've not really found it slow/annoying enough to
specifically seperate tests out in their own commits to facilitate it.

Robbie

On 18 April 2018 at 18:27, Clebert Suconic  wrote:
> I would appreciate if we separated fixes and tests on Pull Requests.
>
>
> A lot of times i will revert the fixes to validate if the test is good
> (if it fails without a fix) and how it failed. (not that I don't trust
> the committer, just part of the validation as sometimes I want to see
> what was the semantic change and fix). I may eventually play a better
> fix in the process.. and I am sure that would apply to anyone else
> helping on reviewing commits.
>
>
> I had at some point gone back in history and needed to apply the test
> without a fix to find a better fix.
>
> I know eventually it's not possible to separate these.. but if you
> could as much as possible separate them:?
>
>
> I recently did that into PR #2004...
>
> https://github.com/apache/activemq-artemis/commit/a72046a0e32fd47cad988a8d71512927f74c8585
>
> https://github.com/apache/activemq-artemis/commit/a72046a0e32fd47cad988a8d71512927f74c8585
>
>
>
>
> I may update the hacking guide with this.. WDYT?
>
>
> --
> Clebert Suconic

[GitHub] activemq-artemis issue #2015: ARTEMIS-1807 File-based Large Message encoding...

[orpiske]

Github user orpiske commented on the issue:

https://github.com/apache/activemq-artemis/pull/2015
  
@franz1981 we can definitely work out some numbers for this!


---

[GitHub] activemq-artemis issue #2015: ARTEMIS-1807 File-based Large Message encoding...

[michaelandrepearce]

Github user michaelandrepearce commented on the issue:

https://github.com/apache/activemq-artemis/pull/2015
  
@mtaylor , ive had an offline discussion with @franz1981 he kindly went 
through in detail this change with me, know i better understand +1 from me. 


---

[GitHub] activemq-artemis issue #2015: ARTEMIS-1807 File-based Large Message encoding...

[franz1981]

Github user franz1981 commented on the issue:

https://github.com/apache/activemq-artemis/pull/2015
  
@mtaylor @michaelandrepearce  Yep and it isn't changing the way the mapped 
journal works: it uses a different code path from the journal, without changing 
the MappedSequentialFile in any of its parts.

I agree that I could provide relative measurements to show which impact it 
has vs the original version: with @orpiske probably we could start providing 
some relative numbers before merging anything. @orpiske wdyt?


---

[GitHub] activemq-artemis issue #2015: ARTEMIS-1807 File-based Large Message encoding...

[mtaylor]

Github user mtaylor commented on the issue:

https://github.com/apache/activemq-artemis/pull/2015
  
@michaelandrepearce If you are using non large messages, and without 
paging, this should have 0 impact.  @franz1981 can you confirm.


---

Re: [DISCUSS] Separate commit for Test and Fix on PRs

[Michael André Pearce]

+1 especially as you’ve explained it will be guidance and not a hard and fast 
rule.

Sent from my iPhone

> On 18 Apr 2018, at 21:06, Clebert Suconic  wrote:
> 
> You can cleanup your commits with a git rebase -i HEAD~
> 
> Squash them accordingly... etc... it's an easy operation.
> 
> Complementing what I said earlier... Many committs are ok.. as long as
> they are semantically correct... Intermediate committs that you did
> along your day should be cleaned up and squashed. that makes reviewing
> a lot easier.
> 
> 
> Even if you are committing directly upstream (you still have that
> right as a committer) I would ask you to review your committs before
> pushing. We like to have the github PRs as a peer review tool.. and
> quite honestly it has stopped even myself from doing damage before. I
> like PRs just because of that.
> 
> 
> 
> On Wed, Apr 18, 2018 at 3:54 PM, Clebert Suconic
>  wrote:
>> I would prefer a squash before merging.. or at least some cleanup
>> before people submit a PR.
>> 
>> When reviewing a PR.. it's almost impossible to make a correlation
>> between the committs...
>> 
>> 
>> example commit1: added a line, commit 2: removed that same line... it
>> would make a PR review useless and more hard.
>> 
>> 
>> I'm trying to make the review of PRs easier.. having a full day of
>> work being sent on a PR as a stream will make the reviewer go through
>> what you tried along the day.
>> 
>> 
>> After all. you should be able to checkout and compile any PR.
>> 
>> 
>> 
>> 
>> Anyways: I'm not asking to add the test separated as a strict rule. if
>> for some reason it became difficult to do it along your PR.. that's
>> fine. (which this will of course only apply for bug fixes).
>> 
>> 
>> 
>> 
>> 
>> On Wed, Apr 18, 2018 at 3:34 PM, Michael André Pearce
>>  wrote:
>>> The only comment here is we will need to stop squashing our commits in 
>>> general eg be happy a pr may have many commits I’m happy to do this btw, eg 
>>> internally in our org we don’t care in fact we prefer the full devs commits 
>>> as then it helps unpick stuff, I found it a shock to be squashing so much 
>>> when I started.
>>> 
>>> but generally when locally working I may commit multiple times a day 
>>> different bits eg rework the fix or enhance the test further, it’s one of 
>>> the big benefits of git for me, but before push and PR I squash the commits 
>>> as up till now as we don’t seem to like to have this history.
>>> 
>>> 
>>> 
>>> Sent from my iPhone
>>> 
 On 18 Apr 2018, at 18:27, Clebert Suconic  
 wrote:
 
 I would appreciate if we separated fixes and tests on Pull Requests.
 
 
 A lot of times i will revert the fixes to validate if the test is good
 (if it fails without a fix) and how it failed. (not that I don't trust
 the committer, just part of the validation as sometimes I want to see
 what was the semantic change and fix). I may eventually play a better
 fix in the process.. and I am sure that would apply to anyone else
 helping on reviewing commits.
 
 
 I had at some point gone back in history and needed to apply the test
 without a fix to find a better fix.
 
 I know eventually it's not possible to separate these.. but if you
 could as much as possible separate them:?
 
 
 I recently did that into PR #2004...
 
 https://github.com/apache/activemq-artemis/commit/a72046a0e32fd47cad988a8d71512927f74c8585
 
 https://github.com/apache/activemq-artemis/commit/a72046a0e32fd47cad988a8d71512927f74c8585
 
 
 
 
 I may update the hacking guide with this.. WDYT?
 
 
 --
 Clebert Suconic
>> 
>> 
>> 
>> --
>> Clebert Suconic
> 
> 
> 
> -- 
> Clebert Suconic