[GitHub] activemq-artemis issue #2020: ARTEMIS-1812 fix for missing (core) messages a...

[stanlyDoge]

Github user stanlyDoge commented on the issue:

https://github.com/apache/activemq-artemis/pull/2020
  
Aha, thanks for explanation!


---

[GitHub] activemq-artemis issue #1793: ARTEMIS-1498: Openwire internal headers should...

[clebertsuconic]

Github user clebertsuconic commented on the issue:

https://github.com/apache/activemq-artemis/pull/1793
  
@michaelandrepearce There is another testsuite just for openwire. 
somethings that we inherited from activemq5.. it's under 
./tests/activemq5-unit-tests


that one was fairly broken. i would need to re-check it.


---

[GitHub] activemq-artemis issue #1793: ARTEMIS-1498: Openwire internal headers should...

[michaelandrepearce]

Github user michaelandrepearce commented on the issue:

https://github.com/apache/activemq-artemis/pull/1793
  
i Got a lot of test failures previous, but it seemed a lot better when I 
ran a suite a few weeks back, is that since his latest changes and work that 
you ran it? I’m just conscious this would be a really bit +100 on the whole 
ActiveMQ 6 thing that we all desperately want


---

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

[Clebert Suconic]

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

[GitHub] activemq-artemis issue #1793: ARTEMIS-1498: Openwire internal headers should...

[clebertsuconic]

Github user clebertsuconic commented on the issue:

https://github.com/apache/activemq-artemis/pull/1793
  
this is a nice start.. but it will require some work... a lot of tests are 
broken... the converters need to be updated... (especially on the openwire 
testsuite).


---

[GitHub] activemq-artemis issue #1793: ARTEMIS-1498: Openwire internal headers should...

[michaelandrepearce]

Github user michaelandrepearce commented on the issue:

https://github.com/apache/activemq-artemis/pull/1793
  
@clebertsuconic you able to give @raisaurabh a pr with what you mean. 
I’ve gone though it myself and in general looks fine from what I can tell, eg 
it has a check to see if it’s of openwire message type and if it is it 
returns it instead of converting 


---

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

[Clebert Suconic]

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

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

[Michael André Pearce]

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

[DISCUSS] Separate commit for Test and Fix on PRs

[Clebert Suconic]

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 pull request #2004: ARTEMIS-1793 fix 'destination-type' STO...

[asfgit]

Github user asfgit closed the pull request at:

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


---

[GitHub] activemq-artemis issue #2020: ARTEMIS-1812 fix for missing (core) messages a...

[clebertsuconic]

Github user clebertsuconic commented on the issue:

https://github.com/apache/activemq-artemis/pull/2020
  
I sent a commit to close this PR. definitely a won't fix.


---

[GitHub] activemq-artemis pull request #2020: ARTEMIS-1812 fix for missing (core) mes...

[asfgit]

Github user asfgit closed the pull request at:

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


---

[GitHub] activemq-artemis issue #2020: ARTEMIS-1812 fix for missing (core) messages a...

[jbertram]

Github user jbertram commented on the issue:

https://github.com/apache/activemq-artemis/pull/2020
  
I think this may be a misunderstanding about how the core client works vs. 
the other protocols. The core client is very simple. It doesn't support 
address/queue auto-create and auto-delete. That functionality is handled by 
each protocol independently. 


---

[GitHub] activemq-artemis issue #2020: ARTEMIS-1812 fix for missing (core) messages a...

[clebertsuconic]

Github user clebertsuconic commented on the issue:

https://github.com/apache/activemq-artemis/pull/2020
  
this is a won't fix.. close it please!


---

[GitHub] activemq-artemis pull request #2020: ARTEMIS-1812 fix for missing (core) mes...

[clebertsuconic]

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

https://github.com/apache/activemq-artemis/pull/2020#discussion_r182439370
  
--- Diff: 
tests/integration-tests/src/test/java/org/apache/activemq/artemis/tests/integration/cluster/failover/FailoverTest.java
 ---
@@ -1641,6 +1642,55 @@ public void 
testFailThenReceiveMoreMessagesAfterFailover() throws Exception {
   receiveDurableMessages(consumer);
}
 
+   @Test(timeout = 12)
+   public void testLiveKilledBackupReceivesCoreMessage() throws Exception {
+  createSessionFactory();
+  SimpleString lalaQ = new SimpleString("lalaQ");
+
+  ClientSession session = createSession(sf, true, true);
+
+  AddressSettings addressSettings = new AddressSettings();
+  addressSettings.setAutoDeleteQueues(true).setAutoCreateQueues(true);
+  
backupServer.getServer().getConfiguration().addAddressesSetting(lalaQ.toString(),
 addressSettings);
+
+  session.createQueue(lalaQ, RoutingType.MULTICAST, lalaQ, null, true);
+
+  ClientProducer producer = session.createProducer(lalaQ);
+
+  producer.send(createMessage(session, 0, true));
+  producer.send(createMessage(session, 1, true));
+
+  ClientConsumer consumer = session.createConsumer(lalaQ, true);
+
+  session.start();
+
+  ClientMessage message = consumer.receive(1000);
+  Assert.assertNotNull(message);
+
+  crash(session);
+
+  message = consumer.receive(1000);
+  Assert.assertNotNull(message);
+  message.acknowledge();
+
+  message = consumer.receive(1000);
+  Assert.assertNotNull(message);
+  message.acknowledge();
+
+  message = consumer.receive(1000);
+  Assert.assertNull(message);
+
+
+  //queue should be destroyed after consuming all messages
+  
backupServer.getServer().getActiveMQServerControl().destroyQueue(lalaQ.toString(),
 true);
--- End diff --

The issue is on auto-created queues?


if you need persistent queues and failover.. create them... I wouldn't fix 
this.


---

[GitHub] activemq-artemis pull request #2020: ARTEMIS-1812 fix for missing (core) mes...

[clebertsuconic]

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

https://github.com/apache/activemq-artemis/pull/2020#discussion_r182438794
  
--- Diff: 
artemis-server/src/main/java/org/apache/activemq/artemis/core/protocol/core/ServerSessionPacketHandler.java
 ---
@@ -658,6 +658,9 @@ private void onSessionSend(Packet packet) {
  try {
 final SessionSendMessage message = (SessionSendMessage) packet;
 requiresResponse = message.isRequiresResponse();
+if 
(this.session.getMatchingQueue(message.getMessage().getAddressSimpleString(), 
RoutingType.ANYCAST) == null || 
this.session.getMatchingQueue(message.getMessage().getAddressSimpleString(), 
RoutingType.MULTICAST) == null) {
--- End diff --

This will kill performance.. we cannot accept this
Matcing Queue on every onSessionSend.. not happening


---

[GitHub] activemq-artemis pull request #2028: ARTEMIS-1653 Allow database tables to b...

[franz1981]

Github user franz1981 closed the pull request at:

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


---

[GitHub] activemq-artemis pull request #2029: ARTEMIS-1806 JDBC Connection leaks

[franz1981]

Github user franz1981 closed the pull request at:

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


---

[GitHub] activemq-artemis pull request #2027: ARTEMIS-1784 JDBC NodeManager should ju...

[asfgit]

Github user asfgit closed the pull request at:

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


---

[GitHub] activemq-artemis issue #2028: ARTEMIS-1653 Allow database tables to be creat...

[clebertsuconic]

Github user clebertsuconic commented on the issue:

https://github.com/apache/activemq-artemis/pull/2028
  
merged.. close it


---

[GitHub] activemq-artemis issue #2020: ARTEMIS-1812 fix for missing (core) messages a...

[stanlyDoge]

Github user stanlyDoge commented on the issue:

https://github.com/apache/activemq-artemis/pull/2020
  
While sending AMQP message, addReceiver() is called, which causes creating 
a queue.
While sending openwire message, addConsumer() is called, which also causes 
creating a queue.
But core message is directly sent to queue. It may happen this queue does 
not exist and message is lost.


---

[GitHub] activemq-artemis issue #2029: ARTEMIS-1806 JDBC Connection leaks

[clebertsuconic]

Github user clebertsuconic commented on the issue:

https://github.com/apache/activemq-artemis/pull/2029
  
merged.. close it


---

[GitHub] activemq-artemis pull request #2024: ARTEMIS-1814 Try original connection wh...

[asfgit]

Github user asfgit closed the pull request at:

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


---

[GitHub] activemq-artemis issue #2027: ARTEMIS-1784 JDBC NodeManager should just use ...

[clebertsuconic]

Github user clebertsuconic commented on the issue:

https://github.com/apache/activemq-artemis/pull/2027
  
These 3 Pull Requests are all about JDBC... it would be a lot easier if you 
posted a single Pull Request with JDBC Changes with 3 commits on it. A lot 
easier to manage


---

[GitHub] activemq-artemis pull request #2029: ARTEMIS-1806 JDBC Connection leaks

[franz1981]

GitHub user franz1981 opened a pull request:

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

ARTEMIS-1806 JDBC Connection leaks

https://issues.apache.org/jira/browse/ARTEMIS-1806

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

$ git pull https://github.com/franz1981/activemq-artemis 1.x_ARTEMIS-1806

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

https://github.com/apache/activemq-artemis/pull/2029.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 #2029


commit 650d4d614c566a04e8783babd9e1302cc0418c36
Author: Francesco Nigro 
Date:   2018-04-14T08:37:34Z

ARTEMIS-1806 JDBC Connection leaks

(cherry picked from commit bbb2f708dd86d681a02237aa1b7c38af37d4)




---

[GitHub] activemq-artemis pull request #2028: ARTEMIS-1653 Allow database tables to b...

[franz1981]

GitHub user franz1981 opened a pull request:

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

 ARTEMIS-1653 Allow database tables to be created externally

https://issues.apache.org/jira/browse/ARTEMIS-1653

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

$ git pull https://github.com/franz1981/activemq-artemis 1.x_ARTEMIS-1653

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

https://github.com/apache/activemq-artemis/pull/2028.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 #2028


commit 24f7caba501c102fcc88d41926ab9d7aefc3e315
Author: Niels Lippke 
Date:   2018-01-28T16:53:35Z

ARTEMIS-1653 Allow database tables to be created externally

(cherry picked from commit eab498456762c3df0f786b1f9ae4e372fdbbfa32)

commit e10391dc885dd74b404d41f04c76d163f1c8227d
Author: Francesco Nigro 
Date:   2018-04-03T08:11:04Z

ARTEMIS-1653 Allow database tables to be created externally

(cherry picked from commit c7651853cdb291dfa3bd2906e1e082fd06cff612)




---

[GitHub] activemq-artemis pull request #2027: ARTEMIS-1784 JDBC NodeManager should ju...

[franz1981]

GitHub user franz1981 opened a pull request:

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

ARTEMIS-1784 JDBC NodeManager should just use DMBS clock

It avoid using the system clock to perform the locks logic
by using the DBMS time.
It contains several improvements on the JDBC error handling
and an improved observability thanks to debug logs.

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

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

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

https://github.com/apache/activemq-artemis/pull/2027.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 #2027


commit 6e9195224c163090e485e0d76c6b4418a583fb75
Author: Francesco Nigro 
Date:   2018-04-04T16:37:12Z

ARTEMIS-1784 JDBC NodeManager should just use DMBS clock

It avoid using the system clock to perform the locks logic
by using the DBMS time.
It contains several improvements on the JDBC error handling
and an improved observability thanks to debug logs.




---

[GitHub] activemq-artemis issue #2025: ARTEMIS-1815 adding exclusive-queue example

[pgfox]

Github user pgfox commented on the issue:

https://github.com/apache/activemq-artemis/pull/2025
  
@clebertsuconic , Thanks Clebert.


---