Bug 329254

Comment 1 Pascal Lehmann 2010-11-02 09:26:12 EDT

Created attachment 182203 [details]
Testcase

Comment 2 Pascal Lehmann 2010-11-02 10:17:41 EDT

Created attachment 182208 [details]
Testcase v2

Testcase & config with proper imports :)

Comment 3 Pascal Lehmann 2010-11-03 07:27:16 EDT

Created attachment 182282 [details]
Patch

I splitted creation and the actual set of the lastCommitTimeStamp into 2 operations, so setLastCommitTimeStamp won't be called when there is an exception thrown earlier in the write method of the TransactionCommitContext.

Comment 4 Eike Stepper 2010-11-06 04:51:29 EDT

Created attachment 182547 [details]
Combined patch

I've fixed the test so tat it passes in legacy configs.

I wonder though whether the delayed setting of the commit timestamp can create new issues with long and short commits at the same time. Can we discuss that next week?

Comment 5 Eike Stepper 2011-01-24 12:37:29 EST

Created attachment 187448 [details]
Patch v3

This patch fixes the problem that clients fill up their outOfOrderInvalidations queue because of missing timestamps. For failed commits we now send a CommitNotificationRequest with a CDOCommitInfo that only contains the two timestamps. Especially the branch is null so that it can be recognized. This commit info "unlocks" the mentioned queue.

Comment 6 Eike Stepper 2011-01-24 12:44:11 EST

Committed revision 6920:
- trunk/plugins/org.eclipse.emf.cdo.server
- trunk/plugins/org.eclipse.emf.cdo
- trunk/plugins/org.eclipse.emf.cdo.common

Comment 7 Eike Stepper 2011-01-24 12:45:44 EST

Keeping bugzilla open because Pascal plans to write a specific tets case...

Comment 8 Eike Stepper 2011-01-25 05:50:20 EST

Created attachment 187507 [details]
Patch B v1

I forgot to apply the same cure to the committing client. Funny this was even harder.

Comment 9 Eike Stepper 2011-01-25 05:50:38 EST

Committed revision 6926:
- trunk/plugins/org.eclipse.emf.cdo
- trunk/plugins/org.eclipse.emf.cdo.net4j
- trunk/plugins/org.eclipse.emf.cdo.server.net4j

Comment 10 Pascal Lehmann 2011-01-25 10:18:35 EST

Created attachment 187521 [details]
Testcase v3

Updated the testcase, holding now the following tests:
- Change on the same object (== Error & timestamp update on repo).
- Long running commit on objects of the same type.
- Long running commit on objects of different type.

The first test fails sometimes, also with the newest patch applied.
The last two always fail.
With my patch they seem to all run fine though.

Comment 11 Eike Stepper 2011-01-25 13:11:39 EST

Created attachment 187551 [details]
Testcase v4 (just reformatted sligtly)

Comment 12 Eike Stepper 2011-01-25 13:14:56 EST

(In reply to comment #10)
> Created attachment 187521 [details]
> Testcase v3
> 
> Updated the testcase, [...]

Updated? I only see a new test class i the patch.

> The first test fails sometimes, also with the newest patch applied.
> The last two always fail.

Yes, for me the last two fail.

> With my patch they seem to all run fine though.

Which one? The one from Nov., 3rd? I can't even apply that one anymore ;-(

Comment 13 Eike Stepper 2011-01-25 13:57:41 EST

*** Bug 334659 has been marked as a duplicate of this bug. ***

Comment 14 Pascal Lehmann 2011-01-26 02:46:53 EST

Created attachment 187613 [details]
Updated patch (separation of timestamp generation & actual set)

(In reply to comment #12)
> (In reply to comment #10)
> Updated? I only see a new test class i the patch.
It was never committed, but there are already 2 versions posted :)

> Which one? The one from Nov., 3rd? I can't even apply that one anymore ;-(
Yes, that one. Interesting as it applies without problems here. I attached an
updated version :)

Comment 15 Pascal Lehmann 2011-01-26 10:49:51 EST

Created attachment 187647 [details]
patch v2

Removed the timestamp reset in case of an error.

Comment 16 Eike Stepper 2011-01-26 14:22:50 EST

Created attachment 187671 [details]
Patch v3 - ready to be committed

I've moved the call to setLastCommitTime() into the updateInfraStructure() method which is guaranteed to be called only after a successful call to accessor.commit().

Comment 17 Eike Stepper 2011-01-26 14:24:18 EST

Pascal, I've not looked again at Testcase v4, please feel free to commit it if it does not harm the test suites ;-)

Comment 18 Pascal Lehmann 2011-01-27 03:19:23 EST

Committed revision 6942:
- trunk/plugins/org.eclipse.emf.cdo.server
- trunk/plugins/org.eclipse.emf.cdo.tests

Comment 19 Pascal Lehmann 2011-01-27 04:03:04 EST

Testcase is included as well.

Comment 20 Pascal Lehmann 2011-01-28 08:36:36 EST

I found a problem in CDOSessionImpl.invalidateOrdered(), so I could move back the timestamp to the createCommitTimestamp where it originally resided.

Committed revision 6958:
- trunk/plugins/org.eclipse.emf.cdo
- trunk/plugins/org.eclipse.emf.cdo.server

Comment 21 Eike Stepper 2011-01-29 02:09:28 EST

Committed revision 6960 (tests)

Comment 22 Pascal Lehmann 2011-01-31 03:18:57 EST

There is still a problem with setting the CTS on the session wich produced the Error. So the test will fail whenever the session which the assert is checked against causes the error in the concurrent commit.

Comment 23 Pascal Lehmann 2011-01-31 03:41:19 EST

Created attachment 187931 [details]
Testcase v5

Updated the testcase, so both directions are checked, no matter which commit thread (session) causes the Error.

Comment 24 Pascal Lehmann 2011-02-03 08:17:23 EST

just noticed that the error test case might pass when one of the commit threads was slow enough to already receive the invalidation of the other: you will receive the following client side error instead of the server error:

org.eclipse.emf.cdo.util.CommitException: This transaction has conflicts

I'll try to update the testcase once again :)

Comment 25 Eike Stepper 2011-02-03 08:33:08 EST

Created attachment 188227 [details]
Patch v4

Suppressing *all* exceptions from write/commit on the server.

Comment 26 Eike Stepper 2011-02-03 08:38:26 EST

Created attachment 188228 [details]
Patch v5

Comment 27 Eike Stepper 2011-02-03 08:50:38 EST

Committed revision 7013

Comment 28 Pascal Lehmann 2011-02-03 11:03:53 EST

Created attachment 188250 [details]
Testcase v6

Fixed the first testcase logic (server side error on concurrent commit).

Comment 29 Eike Stepper 2011-02-03 12:48:11 EST

Thank you Pascal. I've committed it to kick a new build...

BTW, I've changed DEFAULT_TIMEOUT_EXPECTED to DEFAULT_TIMEOUT. Or do we expect a timeout there?

Comment 30 Eike Stepper 2011-02-03 12:48:43 EST

Committed revision 7015

Comment 31 Pascal Lehmann 2011-02-04 02:55:25 EST

(In reply to comment #29)
> BTW, I've changed DEFAULT_TIMEOUT_EXPECTED to DEFAULT_TIMEOUT. Or do we expect
> a timeout there?

Not if the bug is fixed :) But to shorten the test cycles while developping the test I chose to use the 2sec timeout instead of the 2min timeout.

Comment 32 Eike Stepper 2011-06-23 03:42:11 EDT

Available in R20110608-1407