Skip to content

Conversation

rvansa
Copy link
Contributor

@rvansa rvansa commented Jun 8, 2015

This is a follow up to #974

Last commit replaced all CacheKey with Object. Because of statistics, added unwrap operation to all AccessStrategies - this is to be reviewed yet, since I had to modify SessionFactoryImplementor interface.

@rvansa
Copy link
Contributor Author

rvansa commented Jun 8, 2015

Wrt type safety: can I use Java 8 type annotations to do the compile type check?

@rvansa
Copy link
Contributor Author

rvansa commented Jun 11, 2015

@sebersole @Sanne @alexsnaps @galderz Any feedback?

@Sanne
Copy link
Member

Sanne commented Jun 11, 2015

I can't look at it today, will try to make some time tonight or tomorrow at
latest
On 11 Jun 2015 08:20, "Radim Vansa" notifications@github.com wrote:

@sebersole https://github.com/sebersole @Sanne
https://github.com/Sanne @alexsnaps https://github.com/alexsnaps
@galderz https://github.com/galderz Any feedback?


Reply to this email directly or view it on GitHub
#976 (comment)
.

@galderz
Copy link
Member

galderz commented Jun 17, 2015

Wrt type safety: can I use Java 8 type annotations to do the compile type check?

I don't know, @sebersole would be able to say.

One alternative option for additions to EntityRegionAccessStrategy...etc could be:

public <T> T generateCacheKey(Object id, EntityPersister persister, SessionFactoryImplementor factory, String tenantIdentifier);
public <T> T getCacheKeyId(Object cacheKey);

By doing that, if the caller knows what the generated cache key or generated cache key is exactly, it doesn't need to do any casting. If it doesn't know, it can simply assign to object. As far as 2LC implementations is concerned, it would not change anything, we still treat keys as opaque, but maybe for the rest of Hibernate code base it helps?

Otherwise, the rest looks fine to me.

@Sanne
Copy link
Member

Sanne commented Jun 17, 2015

Hi Galder,
the caller (Hibernate ORM internals) doesn't use those objects so the type is not relevant for the caller.
Also, that signature would imply that the 2lc implementation needs to know what kind of key it's expected to create, while the goal is to allow implementors to have more flexibility (i.e. return whatever they see most fit from a performance perspective).

So we'll stick simply with 'Object' as it seems it's a non-problem :)

@galderz
Copy link
Member

galderz commented Jun 26, 2015

@Sanne Fine by me :)

Sanne and others added 4 commits June 30, 2015 12:43
* Replaced all CacheKey with Object
* Because of statistics, added unwrap operation to all AccessStrategies
** This is to be reviewed yet, since I had to modify SessionFactoryImplementor interface
@rvansa
Copy link
Contributor Author

rvansa commented Jul 1, 2015

I have rebased the PR and added commit with checkstyle fixes (can be squashed too).

@Sanne Sanne self-assigned this Jul 1, 2015
@Sanne
Copy link
Member

Sanne commented Jul 1, 2015

merged! thanks a lot @rvansa

@Sanne Sanne closed this Jul 1, 2015
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