Skip to content
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

[3.0] Bug 1343: Remove getStackTrace to fix throughput degredation #1345

Merged
merged 1 commit into from
Nov 12, 2021

Conversation

dazey3
Copy link
Contributor

@dazey3 dazey3 commented Oct 25, 2021

for #1343

Signed-off-by: Joshua Dettinger [email protected]
Signed-off-by: Will Dazey [email protected]

@dazey3 dazey3 requested a review from rfelcman October 25, 2021 22:09
@dazey3
Copy link
Contributor Author

dazey3 commented Oct 26, 2021

@rfelcman
Thank you for reviewing 2.7. Can I get a review approval for 3.0 as well?

@dazey3
Copy link
Contributor Author

dazey3 commented Nov 1, 2021

@rfelcman Does this PR look ok to merge?

@rfelcman
Copy link
Contributor

rfelcman commented Nov 2, 2021

Signed-off-by: Joshua Dettinger <[email protected]>
Signed-off-by: Will Dazey <[email protected]>
@dazey3
Copy link
Contributor Author

dazey3 commented Nov 12, 2021

@rfelcman I'm not sure why this is failing on 3.0 but not 2.7 or master? This change is really small and functionally the same thing. I have a hard time seeing why there would be failures based on THIS commit.

@dazey3
Copy link
Contributor Author

dazey3 commented Nov 12, 2021

@rfelcman

The 4 test failures are in WDF and I have absolutely no knowledge of that code:

Test: org.eclipse.persistence.testing.tests.wdf.jpa1.entitymanager.TestFlush(test-jpa-wdf).testRelationshipToNew
Error Details           flush succeeded although there is a relation to an unmanaged

Test: org.eclipse.persistence.testing.tests.wdf.jpa1.entitymanager.TestGetReference(test-jpa-wdf).testPersist
Error Details           persisting a hollow entity succeeded (should fail)

Test: org.eclipse.persistence.testing.tests.wdf.jpa1.entitymanager.TestGetReference(test-jpa-wdf).testRemoveNonExisting
Error Details           PersistenceException not thrown as expected

Test: org.eclipse.persistence.testing.tests.wdf.jpa1.entitymanager.TestGetReference(test-jpa-wdf).testNegativ
Error Details           no EntityNotFoundException

They appear to be all negative tests expecting failures that didn't happen.

org.eclipse.persistence.testing.tests.wdf.jpa1.entitymanager.TestGetReference.testPersist()

So this test failed because it expected the persist/flush to fail. Why? The test seems like the entity should be persisted just fine. The test is not documented very well on what it is testing. What is a "a hollow entity" a reference to?

@dazey3
Copy link
Contributor Author

dazey3 commented Nov 12, 2021

Perhaps these tests expect to be run in a specific order and have skated by in the past on that? But now they are running in an unexpected order? Looking at some of them, comments appear to indicate the tests do not expect some values to be in the database:

org.eclipse.persistence.testing.tests.wdf.jpa1.entitymanager.TestGetReference.testRemoveNonExisting()

    Employee emp = em.getReference(Employee.class, Integer.valueOf(99)); // versioning, entity does not exist

But if the entity DOES exist, then the test doesn't fail and other tests ARE persisting the same ID:
org.eclipse.persistence.testing.tests.wdf.jpa1.entitymanager.TestGetReference.testPersist():

Employee emp = null;
            boolean operationFailed = false;
            env.beginTransaction(em);
            try {
                emp = em.getReference(Employee.class, Integer.valueOf(99));
            } catch (EntityNotFoundException e) {
                // $JL-EXC$ expected behavior
                operationFailed = true;
            }
            env.rollbackTransactionAndClear(em);

            if (emp != null) {
                env.beginTransaction(em);
                try {
                    em.persist(emp);
                    em.flush();
                } catch (PersistenceException e) {
                    // $JL-EXC$ expected behavior
                    operationFailed = true;
                }
                env.rollbackTransactionAndClear(em);
                verify(operationFailed, "persisting a hollow entity succeeded (should fail)");
            }

It seems like if testPersist() doesnt fail, then testRemoveNonExisting() has to fail. Bad testing.

@dazey3
Copy link
Contributor Author

dazey3 commented Nov 12, 2021

In any case, I don't think any of these test failures are related to this PR and should be overlooked. I would like to get this change merged so that 3.0 can match 2.7 and master in ConcurrencyManager

Copy link
Contributor

@rfelcman rfelcman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@rfelcman rfelcman merged commit cbd807c into eclipse-ee4j:3.0 Nov 12, 2021
@lukasj
Copy link
Member

lukasj commented Nov 12, 2021

In any case, I don't think any of these test failures are related to this PR and should be overlooked

that happens to me from time to time and I go ahead and fix those failures - setting specific order is not that difficult change to make. What have you done to make sure failures you see will be addressed?

@dazey3 dazey3 deleted the 1343_3.0 branch November 15, 2021 16:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants