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

ISPN-1496 - FineGrainedAtomicMap not working with REPEATABLE_READ #615

Closed
wants to merge 1 commit into from

Conversation

vblagoje
Copy link

@vblagoje vblagoje commented Nov 4, 2011

@Sanne
Copy link
Member

Sanne commented Nov 5, 2011

Hi,
the tests I've added are important for other projects to work fine, we really can't disable existing tests to move forward on a different issue. OGM is relying on the correct working of FGAM, so please be carefull...

For OGM's purpose it would be much better to wait longer on repeatable_read than to break the default isolation level.

@maniksurtani
Copy link
Member

Vladimir, also you may want to hold back for my pull request - #614 - to get merged, since it does change some of the AtomicHashMap code. It shouldn't affect your stuff here, but you should rebase and test just in case.

@maniksurtani
Copy link
Member

And on a related note, if either of you want to review and handle my pull req that'd be great. :)

@galderz
Copy link
Member

galderz commented Nov 7, 2011

Manik, your pull req is in.

@maniksurtani
Copy link
Member

Yup I saw the notifications. Thanks!

Sent via the InterWebs

On 7 Nov 2011, at 09:14, Galder Zamarreñoreply@reply.github.com wrote:

Manik, your pull req is in.


Reply to this email directly or view it on GitHub:
#615 (comment)

@galderz
Copy link
Member

galderz commented Nov 7, 2011

In fact, Manik's changes introduces some test failures, particularly around the fine grained atomic map stuff:
https://infinispan.ci.cloudbees.com/job/Infinispan-master-JDK6-tcp/291/#showFailuresLink

@Sanne
Copy link
Member

Sanne commented Nov 7, 2011

@galderz, why was it merged then?

@galderz
Copy link
Member

galderz commented Nov 7, 2011

Cos the code looked fine :)

To be clear, I'm not gonna be running the testsuite for other people, individuals need to do it when they send pull reqs :). Sanne, I know you sometimes (always?) do so, but I refuse to do so.

@Sanne
Copy link
Member

Sanne commented Nov 7, 2011

Yes I do it almost all the time, looking at the code is not enough, I think it's important but agree with your position as it's a huge time sink and we should find a better process.
It would be nice if sending a pull request could kick in Jenkins and have the pull request auto-rejected on failing tests (not going to work until all tests are consistently fine).

We should find a good compromise; like if all of you could use emmanuel's build script, at least we would know that when the original developer has run his tests, it has run them on his commit and not on his workspace. They can be different. Also, to give an example the other day I spotted a compile failure on Mircea's code as he was using a case insensitive filesystem.

@maniksurtani
Copy link
Member

+1 to using Emmanuel's build script. @Sanne - are instructions on the "Contributing to Infinispan" page?

In the meanwhile I'll investigate what we can do with Jenkins/CloudBees to kick off runs on developer topic branches when pull requests are sent in.

@maniksurtani
Copy link
Member

As for my patch breaking FGAM, I'll issue another pull req shortly with a fix.

@vblagoje
Copy link
Author

vblagoje commented Nov 7, 2011

On 11-11-07 7:48 AM, Manik Surtani wrote:

As for my patch breaking FGAM, I'll issue another pull req shortly with a fix.


Reply to this email directly or view it on GitHub:
#615 (comment)
I've done something similar a while ago and ever since I have developed
a slight phobia of screwing up infinispan build and that is a good thing
IMO - I've never done it again, I always run the entire testsuite :-)

@galderz
Copy link
Member

galderz commented Nov 7, 2011

Still don't get why we'd need to use Emmanuel's script. We have CI for a reason, to catch these things. So, if we spot that a pull req has resulted in a regression, just raise it up and that's it, like I did here. No reason to panic IMO but keep an eye on CI.

Having to execute Emmanuel's script is duplicating the work that CI does.

@Sanne
Copy link
Member

Sanne commented Nov 8, 2011

perfect, merged! thanks

@Sanne Sanne closed this Nov 8, 2011
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