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.

Sign up for GitHub

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

Jump to bottom

WFLY-12310 Added Second Level Cache Tests #12444

Merged
merged 1 commit into from
Aug 8, 2019
Merged

WFLY-12310 Added Second Level Cache Tests #12444

merged 1 commit into from
Aug 8, 2019

Conversation

tommaso-borgato
Copy link
Contributor

@tommaso-borgato tommaso-borgato commented Jul 22, 2019 •
edited
Loading

@wildfly-ci wildfly-ci added the deps-ok Dependencies have been checked, and there are no significant changes label Jul 22, 2019
simkam
simkam suggested changes Jul 22, 2019
View reviewed changes
Copy link
Contributor

@simkam simkam left a comment

Choose a reason for hiding this comment

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

requested some minor changes, otherwise it looks good to me

...st/java/org/jboss/as/test/clustering/cluster/jpa2lc/ClusteredJPA2LCCustomRegionTestCase.java Outdated Show resolved Hide resolved
...st/java/org/jboss/as/test/clustering/cluster/jpa2lc/ClusteredJPA2LCCustomRegionTestCase.java Outdated Show resolved Hide resolved
...st/java/org/jboss/as/test/clustering/cluster/jpa2lc/ClusteredJPA2LCCustomRegionTestCase.java Outdated Show resolved Hide resolved
...st/java/org/jboss/as/test/clustering/cluster/jpa2lc/ClusteredJPA2LCCustomRegionTestCase.java Outdated Show resolved Hide resolved
...st/java/org/jboss/as/test/clustering/cluster/jpa2lc/ClusteredJPA2LCCustomRegionTestCase.java Outdated Show resolved Hide resolved
...st/java/org/jboss/as/test/clustering/cluster/jpa2lc/ClusteredJPA2LCCustomRegionTestCase.java Outdated Show resolved Hide resolved
...st/java/org/jboss/as/test/clustering/cluster/jpa2lc/ClusteredJPA2LCCustomRegionTestCase.java Outdated Show resolved Hide resolved
...st/java/org/jboss/as/test/clustering/cluster/jpa2lc/ClusteredJPA2LCCustomRegionTestCase.java Outdated Show resolved Hide resolved
...st/java/org/jboss/as/test/clustering/cluster/jpa2lc/ClusteredJPA2LCCustomRegionTestCase.java Outdated Show resolved Hide resolved
...ering/src/test/java/org/jboss/as/test/clustering/cluster/jpa2lc/DummyEntityCustomRegion.java Outdated Show resolved Hide resolved
darranl
darranl requested changes Jul 22, 2019
View reviewed changes
Copy link
Contributor

@darranl darranl left a comment

Choose a reason for hiding this comment

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

Can you please create a WFLY issue for these changes and reference this instead of the EAP7 issue in the commit message and PR title.

As an open source project we should be using Jira issues from the projects public issue tracking tool.

@tommaso-borgato tommaso-borgato changed the title WFLY-18.0.0.Beta1-SNAPSHOT Added Second Level Cache for EAP7-982 WFLY-12310 Added Second Level Cache for EAP7-982 Jul 22, 2019
darranl
darranl requested changes Jul 23, 2019
View reviewed changes
Copy link
Contributor

@darranl darranl left a comment

Choose a reason for hiding this comment

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

The reference to the WFLY issue looks good but please loose the EAP7 issue entirely, those are not community visible.

@tommaso-borgato tommaso-borgato changed the title WFLY-12310 Added Second Level Cache for EAP7-982 WFLY-12310 Added Second Level Cache Tests Jul 23, 2019
@tommaso-borgato tommaso-borgato force-pushed the second-level-cache-tests branch 2 times, most recently from 77344c6 to 235c330 Compare July 23, 2019 13:18
@scottmarlow
Copy link
Contributor

I looked at the Linux JDK 8 failures yesterday and they don't appear related to this change.

@tommaso-borgato
Copy link
Contributor Author

@scottmarlow is there anything I forgot do to make this PR ready to be merged?

@rhusar
Copy link
Member

rhusar commented Jul 26, 2019

retest this please

@tommaso-borgato
Copy link
Contributor Author

how to do the retest?

@simkam
Copy link
Contributor

simkam commented Jul 30, 2019

@tommaso-borgato "retest this please" is the signal for CI bot to rerun tests on this PR, all passed

@scottmarlow
Copy link
Contributor

I'm fine with this change getting merged, I was going to look more closely but no need to block on me. Thanks for improving the 2lc tests!

@mnovak1
Copy link
Contributor

mnovak1 commented Aug 6, 2019

@scottmarlow hi, @tommaso-borgato is on 2 week PTO and I'm reviewing RFEs after him. Based on the coments above this one is ok to merge, right?

@kabir
Copy link
Contributor

kabir commented Aug 6, 2019

Retest this please

@kabir kabir added the ready-for-merge Only for use by those with merge permissions! label Aug 6, 2019
kabir
kabir approved these changes Aug 6, 2019
View reviewed changes
Copy link
Contributor

@kabir kabir left a comment

Choose a reason for hiding this comment

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

From the discussion, this looks ready for merge once the retest passes

@scottmarlow
Copy link
Contributor

@mnovak1 yes, it just needs to pass tests which shouldn't be a problem as it passed locally for me.

jamezp added a commit to jamezp/wildfly that referenced this pull request Aug 7, 2019
@jamezp
Merged pull request wildfly#12444
18e7cac 
Added Second Level Cache Tests (WFLY-12310)
@jamezp jamezp mentioned this pull request Aug 7, 2019
Closed
@mnovak1
Copy link
Contributor

mnovak1 commented Aug 8, 2019

@scottmarlow @kabir great, thanks

@kabir kabir merged commit f7600f4 into wildfly:master Aug 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@scottmarlow scottmarlow scottmarlow left review comments

@zroubalik zroubalik zroubalik left review comments

@darranl darranl darranl requested changes

@kabir kabir kabir approved these changes

@simkam simkam simkam approved these changes

Assignees
No one assigned
Labels
deps-ok Dependencies have been checked, and there are no significant changes ready-for-merge Only for use by those with merge permissions!
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@tommaso-borgato @scottmarlow @rhusar @simkam @mnovak1 @kabir @darranl @zroubalik @wildfly-ci