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-3400 Upgrade to JSR-107 (JCache) 1.0.0-PFD version #2187

Closed
wants to merge 1 commit into from

Conversation

galderz
Copy link
Member

@galderz galderz commented Nov 1, 2013

  • Upgraded to Public Final Draft version of the JSR-107 specification.
  • TCK has expanded loadAll() and expiry policy usage tests, which in turn has required more complex logic to deal with all the expectations in the TCK. For example, the logic for loadAll() is now different based on whether a JCache cache loader has been configured, or the Infinispan configuration has a native cache loader configured.
  • Tweaked a test util method to provide more generic friendly returns that avoids the need for cast.

@galderz
Copy link
Member Author

galderz commented Nov 6, 2013

This needs some serious rebasing...

@galderz
Copy link
Member Author

galderz commented Nov 6, 2013

Rebased and tests are passing :)

@@ -246,7 +244,7 @@ private boolean shouldInvoke(Object event, boolean isLocalNodePrimaryOwner) {
private Throwable getRealException(Throwable re) {
if (re.getCause() == null) return re;
Throwable cause = re.getCause();
if (cause instanceof CacheException || cause instanceof RuntimeException)
if (cause instanceof CacheException || cause instanceof RuntimeException || cause instanceof Error)
Copy link
Contributor

Choose a reason for hiding this comment

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

CacheException instanceOf RuntimeException, I guess you can simplify here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, it can be simplified.

@mmarkus
Copy link
Contributor

mmarkus commented Nov 7, 2013

@galderz looks good to me. Needs rebase though.
@vblagoje might want to take another look?

try {
return policy.getExpiryForCreation();
} catch (Throwable t) {
return Duration.ETERNAL;
Copy link

Choose a reason for hiding this comment

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

Why Duration.Eternal here and null in other cases?

Copy link
Member Author

Choose a reason for hiding this comment

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

It should probably be Eternal in all, let me try...

Copy link
Member Author

Choose a reason for hiding this comment

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

I've opened an issue in the TCK. The TCK expects that if access or modified expiration callbacks throw exceptions, the entries are left untouched (as opposed to what happens with creation time), so the code just matches what the TCK expects here. Let's see how the issue is resolved and change afterwards if necessary.

@vblagoje
Copy link

vblagoje commented Nov 8, 2013

This is a big freaking PR. I've tried to look for fishy things from the outset as much as possible. Hope you found it helpful Galder

@galderz
Copy link
Member Author

galderz commented Nov 11, 2013

Updated PR addressing comments made.

@galderz
Copy link
Member Author

galderz commented Nov 13, 2013

I've rebased now.

@vblagoje
Copy link

Mircea would you please integrate this PR? I don't have a fresh infinispan source tree as I have spent most of the time on hrcpp. Cheers

* Upgraded to Public Final Draft version of the JSR-107 specification.
* TCK has expanded loadAll() and expiry policy usage tests, which in
turn has required more complex logic to deal with all the expectations
in the TCK. For example, the logic for loadAll() is now different based
on whether a JCache cache loader has been configured, or the Infinispan
configuration has a native cache loader configured.
* Tweaked a test util method to provide more generic friendly returns
that avoids the need for cast.
@galderz
Copy link
Member Author

galderz commented Nov 13, 2013

It's rebased again now.

@mmarkus
Copy link
Contributor

mmarkus commented Nov 13, 2013

@vblagoje sure I will :-)
get a ISPN source tree when you have the time, reviewing should be handled by all of us ;)

@mmarkus
Copy link
Contributor

mmarkus commented Nov 13, 2013

@galderz integrated, thanks!

@mmarkus mmarkus closed this Nov 13, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants