Do not merge - This is a draft fix for https://jira.duraspace.org/browse... #693
Conversation
| @@ -264,7 +264,7 @@ public void testCreateTwoVersionsWithSameLabel() throws Exception { | |||
| new HttpGet(serverAddress + objId + "/fcr:versions"); | |||
| logger.debug("Retrieved versions"); | |||
| final GraphStore results = getGraphStore(getVersions); | |||
| assertEquals("Expected exactly 3 triples!", 3, countTriples(results)); | |||
| assertEquals("Expected exactly 3 triples!", 5, countTriples(results)); | |||
There was a problem hiding this comment.
I do not expect this PR necessarily go through, as creating version checkpoints when adding the duplicate label would cause the operation to fail does not seem right... but if this PR did go through, yes, the assertion text should be updated to "Expected exactly 5 triples!"
Thoughts? I have also asked for @mikedurbin's feedback.
There was a problem hiding this comment.
Maybe leave a note explaining why exactly X triples? Otherwise it's an unpleasantly magical number.
|
I agree, that if the operation fails (adding a new version with a label) it should fail completely. The current code (without the PR) does that in all cases except some almost impossibly unlikely concurrent updates, while the PR would result in an unlabelled version checkpoint being made whenever an attempt at label reuse was made. Neither option is terrible, but I vote for NOT merging this PR and having it fail first before making any version checkpoint. Modeshape's fix is still an improvement in the (incredibly unlikely) case that a concurrent thread adds another version with the same label between when this thread creates a version and attaches the label. (Perhaps we should synchronize that block... though if we are concerned about those sorts of concurrency issues we should look over the whole codebase and take a consistent approach.) |
|
Thanks, @mikedurbin. I agree that the best option is likely to do nothing differently from what we are currently... and close this PR. |
.../FCREPO-1253