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

[6.5.x] RHBPMS-5154: Test case for DisabledFollowOnLockOracle10gDialect #1193

Closed
wants to merge 3 commits into from
Closed

[6.5.x] RHBPMS-5154: Test case for DisabledFollowOnLockOracle10gDialect #1193

wants to merge 3 commits into from

Conversation

martinweiler
Copy link
Contributor

No description provided.

@mswiderski
Copy link
Contributor

Jenkins retest this

@mswiderski mswiderski changed the title RHBPMS-5154: Test case for DisabledFollowOnLockOracle10gDialect [6.5.x] RHBPMS-5154: Test case for DisabledFollowOnLockOracle10gDialect Apr 26, 2018
public class ExecutorQueryServiceTest {

protected ExecutorService executorService;
private static final int THREADS = 2;
Copy link
Member

Choose a reason for hiding this comment

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

Just one question. Are 2 threads enough? I personally would increase it to at least 4. But just a suggestion :) Wdyt @mswiderski.

try {
request = ctx.find(org.jbpm.executor.entities.RequestInfo.class, ((Number)lockedRequest[0]).longValue());
} catch(NumberFormatException nfe) {
}
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we include there at least a warning log message? Empty catch blocks are really not a good practice even if we know that there is a very little chance of throwing that exception.

@mareknovotny
Copy link
Member

@MarianMacik do you still insist on your comments before merging, right?

@MarianMacik
Copy link
Member

@mareknovotny Well, at least I would like to hear opinion on that from @martinweiler. Wdyt?

@martinweiler
Copy link
Contributor Author

@MarianMacik - 2 threads are ok to reproduce the issue. For the NFE, I have added some logging.

* -Dmaven.hibernate.dialect=org.hibernate.dialect.Oracle10gDialect
*
* // the test should be successful using the custom dialect
* -Dmaven.hibernate.dialect=org.jbpm.persistence.jpa.hibernate.DisabledFollowOnLockOracle10gDialect
Copy link
Member

Choose a reason for hiding this comment

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

Thanks, @martinweiler. Just one last question - the issue was with the DisabledFollowOnLockOracle10gDialect not working properly, but Oracle10gDialect working as intended? Because the JavaDoc says that it should fail with org.hibernate.dialect.Oracle10gDialect. Is this still true?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

org.hibernate.dialect.Oracle10gDialect is not working as intended, as it does not lock a job properly. You will see 'HHH000444: Encountered request for locking however dialect reports that database prefers locking be done in a separate select' WARN messages, and the test will fail as the job is executed on both threads.

Copy link
Member

Choose a reason for hiding this comment

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

Then I see one issue with the test. When we run full certification matrix of jbpm, we use Oracle10gDialect which is the first to be supported afaik. Is this DisabledFollowOnLockOracle10gDialect supported too? Anyway, this means that our certification matrix will fail with the Oracle database. So do we still have a bug with default Oracle10gDialect or this use case is not supported with the default Oracle10gDialect?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess then we need to disable this test in the default test suite. See https://bugzilla.redhat.com/show_bug.cgi?id=1234592 as well as https://hibernate.atlassian.net/browse/HHH-1168?focusedCommentId=48846 for background information on why this custom dialect was necessary to begin with.

Copy link
Member

Choose a reason for hiding this comment

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

@martinweiler Are you going to disable the test in this PR then?

@mareknovotny
Copy link
Member

Merged as d1d20e7 and added additional @Ignore in 2816ff7

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants