Re: Adding check on Replicated classes

[max....@gmail.com]

Great start Danny! Mostly style nits, but one big question:
Is this CL supposed to contain the logic that restricts the persistence
of replicated entities to a single shard, or are you saving that for a
separate CL? I'm ok with a separate CL, I just want to make sure I
understand where this is going.

Thanks,
Max

http://codereview.appspot.com/8456/diff/1/22
File java/org/hibernate/shards/ShardedConfiguration.java (right):

http://codereview.appspot.com/8456/diff/1/22#newcode201
Line 201: * Specifically there should be no cascades from classes that
are not replicated, and classes that are replicated.
Please explain why such cascades are problematic.

http://codereview.appspot.com/8456/diff/1/22#newcode207
Line 207: Iterator<PersistentClass> persistentClasses =
(Iterator<PersistentClass>)
this.prototypeConfiguration.getClassMappings();
nit: no need for 'this' prefix. We just do this in constructors and
setters when there are params with the same names as the member.

http://codereview.appspot.com/8456/diff/1/22#newcode211
Line 211: if (psClass.isAnnotationPresent(Replicated.class)) continue;
Please use curly braces even for one line conditionals, and put the
'continue' on its own line.

http://codereview.appspot.com/8456/diff/1/22#newcode218
Line 218: if (cascade==null || cascade.equalsIgnoreCase("NONE"))
continue;
see above style comment

http://codereview.appspot.com/8456/diff/1/22#newcode218
Line 218: if (cascade==null || cascade.equalsIgnoreCase("NONE"))
continue;
Is there really no constant in Hibernate core that we can refer to
instead of the hard-coded 'NONE' string?

http://codereview.appspot.com/8456/diff/1/22#newcode221
Line 221: throw new InvalidMappingException("Illegal cascade from
non-replicated class(property="+ps.getClassName()+") to replilcated
class("+property.getName()+")",psClass.toString(), (String)null);
replilcated -> replicated

http://codereview.appspot.com/8456/diff/1/22#newcode221
Line 221: throw new InvalidMappingException("Illegal cascade from
non-replicated class(property="+ps.getClassName()+") to replilcated
class("+property.getName()+")",psClass.toString(), (String)null);
If you just throw MappingException you can skip the unused param at the
end. Also, I don't think property.getName() returns a class. Ideally
this message would include the name of the non-replicated class, the
name of the replicated class, and the name of the property on the
replicated class that is of the type of the replicated class.

http://codereview.appspot.com/8456/diff/1/24
File java/org/hibernate/shards/strategy/selection/Replicated.java
(right):

http://codereview.appspot.com/8456/diff/1/24#newcode1
Line 1: package org.hibernate.shards.strategy.selection;
Please add the apache license to the top. Just look at the other files
(you can omit Copyright Google).

http://codereview.appspot.com/8456/diff/1/24#newcode12
Line 12: * User: dantonetti
Please add an author tag like the other files have. Your choice whether
to put your email address or not.

http://codereview.appspot.com/8456/diff/1/25
File java/org/hibernate/shards/util/Lists.java (right):

http://codereview.appspot.com/8456/diff/1/25#newcode31
Line 31: public class Lists {
extra space

http://codereview.appspot.com/8456/diff/1/25#newcode55
Line 55: * @return
Please provide a value for @return.

http://codereview.appspot.com/8456/diff/1/3
File
test/org/hibernate/shards/integration/model/ModelCriteriaPermutedIntegrationTest.java
(right):

http://codereview.appspot.com/8456/diff/1/3#newcode61
Line 61: s = ModelDataFactory.state("s1");
indentation. we use 2 spaces, no tabs

http://codereview.appspot.com/8456/diff/1/2
File test/org/hibernate/shards/integration/model/ModelDataFactory.java
(right):

http://codereview.appspot.com/8456/diff/1/2#newcode129
Line 129: public static City city(String name, State s) {
spacing

http://codereview.appspot.com/8456/diff/1/11
File test/org/hibernate/shards/model/City.java (right):

http://codereview.appspot.com/8456/diff/1/11#newcode1
Line 1: package org.hibernate.shards.model;
apache license

http://codereview.appspot.com/8456/diff/1/11#newcode9
Line 9: * Created by IntelliJ IDEA.
Please change to match other shards source files

http://codereview.appspot.com/8456/diff/1/11#newcode16
Line 16: public class City {
indentation

http://codereview.appspot.com/8456/diff/1/10
File test/org/hibernate/shards/model/State.java (right):

http://codereview.appspot.com/8456/diff/1/10#newcode1
Line 1: package org.hibernate.shards.model;
apache license

http://codereview.appspot.com/8456/diff/1/10#newcode10
Line 10: * User: dantonetti
please change to match other shards source files

http://codereview.appspot.com/8456/diff/1/10#newcode17
Line 17: private String name;
indentation

http://codereview.appspot.com/8456/diff/1/18
File
test/org/hibernate/shards/testmodel/replicationtest/NotReplicated.java
(right):

http://codereview.appspot.com/8456/diff/1/18#newcode1
Line 1: package org.hibernate.shards.testmodel.replicationtest;
apache license

http://codereview.appspot.com/8456/diff/1/18#newcode11
Line 11: * User: dantonetti
please change to match other shards source files

http://codereview.appspot.com/8456/diff/1/18#newcode19
Line 19: @Id
indentation

http://codereview.appspot.com/8456/diff/1/16
File
test/org/hibernate/shards/testmodel/replicationtest/NotReplicatedBadCascade.java
(right):

http://codereview.appspot.com/8456/diff/1/16#newcode1
Line 1: package org.hibernate.shards.testmodel.replicationtest;
apache license

http://codereview.appspot.com/8456/diff/1/16#newcode12
Line 12: * User: dantonetti
please change to match other shards source files

http://codereview.appspot.com/8456/diff/1/16#newcode19
Line 19: @Id
indentation

http://codereview.appspot.com/8456/diff/1/19
File
test/org/hibernate/shards/testmodel/replicationtest/Replicated1.java
(right):

http://codereview.appspot.com/8456/diff/1/19#newcode1
Line 1: package org.hibernate.shards.testmodel.replicationtest;
apache license

http://codereview.appspot.com/8456/diff/1/19#newcode12
Line 12: * User: dantonetti
please change to match other shards source files

http://codereview.appspot.com/8456/diff/1/19#newcode20
Line 20: @Id
indentation

http://codereview.appspot.com/8456/diff/1/20
File
test/org/hibernate/shards/testmodel/replicationtest/Replicated2.java
(right):

http://codereview.appspot.com/8456/diff/1/20#newcode1
Line 1: package org.hibernate.shards.testmodel.replicationtest;
apache license

http://codereview.appspot.com/8456/diff/1/20#newcode10
Line 10: * User: dantonetti
please change to match other shards source files

http://codereview.appspot.com/8456/diff/1/20#newcode19
Line 19: @Id
indentation

http://codereview.appspot.com/8456

[Danny Antonetti]

On Sun, Nov 16, 2008 at 9:58 PM, <max....@gmail.com> wrote:

Great start Danny!  Mostly style nits, but one big question:
Is this CL supposed to contain the logic that restricts the persistence
of replicated entities to a single shard, or are you saving that for a
separate CL?  I'm ok with a separate CL, I just want to make sure I
understand where this is going.

Thanks,
Max

I was going to do that logic after you met with Emmanuel.
He thought that our solution was not very good, and thought the XA solution was better.
But never answered my questions, so I wanted to get my questions answered, before did that part.

Is there a style guide for shards?

Thanks,


Danny
 

[Max Ross]

Had a good talk with Emmanuel today.  I'll write it all up in the next few days so we can discuss how to move forward.

[Max Ross]

Happy Thanksgiving!

Emmanuel (cc-ed) and I had a good long talk about our options here (Emmanuel, please jump in if I've missed or misrepresented anything)(.  You're right, implementing real replication support in shards is by no means straightforward, even if we make use of Hibernate's existing XA support and require the user to propertly set it up.  There are a number of scenarios to consider. 

First, as we already identified, any cascade from a non-replicated to a replicated entity is problematic.  The reason is that cascades happen on a specific Session (not a ShardedSession) that is associated with a specific SessionFactory (not a ShardedSessionFactory).  If we were to support this we would first need to detect the cascade.  I don't know exactly how we could do this but let's assume for a moment it's possible.  We would probably need an interceptor on the Session to delegate the cascade back out to the ShardedSessionFactory.  This is doable, but it's unclear how well it would work.  With an interceptor, the cascade will still proceed on the specific Shard with which the parent was associated, so the replication would need to avoid this shard.  The other option is to provide our own Event implementations so that we could avoid processing the cascade and then let replication take care of it for all Shards.  Having implemented a custom Event in the past, I think this is something we should try to avoid.

Second, lookups (fetches of individual objects and queries) are problematic.  Suppose we have a Customer object and a Country object.  Country is replicated.  We have an unowned, one to many relationship between Customer and Country.  Customer 1 lives on Shard 1.  Customer 2 lives on Shard 2.  Both Customer 1 and 2 are associated with USA.  We load Customer 1 from the db and access its Country object via the returned object.  We load Customer 2 from the db.  We update the Country object associated with Customer 1.  The problem is that the Country object associated with Customer 2 will not reflect that change.  This is not the behavior that Hibernate users expect.  One solution is to implement an interceptor that always loads replicated entities from the same shard.  I don't know for sure if this is possible or how it would work, but it does address the above issue.

So, since the goal is to add _basic_ replication support, let's see if we can find a way through these constraints that still delivers something useful to users.  I think the check you've implemented on cascades needs to stay no matter what.  Cascading from a non-replicated to a replicated-entity basically requires us to solve the cross-entity relationship problem, and we ruled that out of scope for v1 long long ago.  I still think that's the right decision.  I think it makes sense to investigate implementing an Interceptor that detects lookups of replicated entities and performs that lookup on a single, well-known shard.  Interceptor.onLoad _seems_ like the logical place for this, except I don't think you can actually change the return value of the load so you'll probably need to perform the lookup on the replication source shard (yes, I'm coining terms on the fly) and then hide the result in some well known location for the ShardedSession to pick up.  If you can get that working then a different interceptor that broadcasts modifications of replicated entities to all shards should be somewhat straightforward.

Does this sound like a reasonable approach to you?

Thanks,
Max

[Emmanuel Bernard]

OK Happy thanksgiving too... sorry for the small delay. I am catching up on emails ;)

A few key questions would benefit from Steve's wisdom. Maybe we can organize some IRC meet-up.

On  Nov 27, 2008, at 20:41, Max Ross wrote:

Happy Thanksgiving!

Emmanuel (cc-ed) and I had a good long talk about our options here (Emmanuel, please jump in if I've missed or misrepresented anything)(.  You're right, implementing real replication support in shards is by no means straightforward, even if we make use of Hibernate's existing XA support and require the user to propertly set it up.  There are a number of scenarios to consider.  

First, as we already identified, any cascade from a non-replicated to a replicated entity is problematic.  The reason is that cascades happen on a specific Session (not a ShardedSession) that is associated with a specific SessionFactory (not a ShardedSessionFactory).  If we were to support this we would first need to detect the cascade.  I don't know exactly how we could do this but let's assume for a moment it's possible. 

I would not mind if cascade was disallowed for non replicated to replicated objects. It's not too common or sensitive to use cascade between these kind of entities.

BTW cascade on the other side is problematic too.

We would probably need an interceptor on the Session to delegate the cascade back out to the ShardedSessionFactory.  This is doable, but it's unclear how well it would work.  With an interceptor, the cascade will still proceed on the specific Shard with which the parent was associated, so the replication would need to avoid this shard. 

This probably could work. You need to let the "original" shard to push the change though to avoid missing FK issues.

The other option is to provide our own Event implementations so that we could avoid processing the cascade and then let replication take care of it for all Shards.  Having implemented a custom Event in the past, I think this is something we should try to avoid.

-1 see above

Second, lookups (fetches of individual objects and queries) are problematic.  Suppose we have a Customer object and a Country object.  Country is replicated.  We have an unowned, one to many relationship between Customer and Country.  Customer 1 lives on Shard 1.  Customer 2 lives on Shard 2.  Both Customer 1 and 2 are associated with USA.  We load Customer 1 from the db and access its Country object via the returned object.  We load Customer 2 from the db.  We update the Country object associated with Customer 1.  The problem is that the Country object associated with Customer 2 will not reflect that change.  This is not the behavior that Hibernate users expect.  One solution is to implement an interceptor that always loads replicated entities from the same shard.  I don't know for sure if this is possible or how it would work, but it does address the above issue.

You might risk to associate the referenced object to multiple sessions. If it happens to have a collection, Hibernate Core will break (not sure about proxy). This might be something we can work on in Core though.

Intercepting the load might be quite tricky though. Steve can help here.

So, since the goal is to add _basic_ replication support, let's see if we can find a way through these constraints that still delivers something useful to users.  I think the check you've implemented on cascades needs to stay no matter what.  Cascading from a non-replicated to a replicated-entity basically requires us to solve the cross-entity relationship problem, and we ruled that out of scope for v1 long long ago.  I still think that's the right decision.  I think it makes sense to investigate implementing an Interceptor that detects lookups of replicated entities and performs that lookup on a single, well-known shard.  Interceptor.onLoad _seems_ like the logical place for this, except I don't think you can actually change the return value of the

right Interceptor.onLoad() is actually called before the load operation is performed and only takes care of state[] changes.

load so you'll probably need to perform the lookup on the replication source shard (yes, I'm coining terms on the fly) and then hide the result in some well known location for the ShardedSession to pick up.  If you can get that working then a different interceptor that broadcasts modifications of replicated entities to all shards should be somewhat straightforward.

I don't understand :)