Bug 213402 - Support external references
Summary: Support external references
Status: CLOSED FIXED
Alias: None
Product: EMF
Classification: Modeling
Component: cdo.core (show other bugs)
Version: 2.0   Edit
Hardware: PC Windows XP
: P5 enhancement (vote)
Target Milestone: M3   Edit
Assignee: Simon Mc Duff CLA
QA Contact:
URL:
Whiteboard: Appealing to a Broader Community
Keywords:
Depends on:
Blocks: 246705 249610 249611
  Show dependency tree
 
Reported: 2007-12-19 00:33 EST by Eike Stepper CLA
Modified: 2010-06-29 09:23 EDT (History)
2 users (show)

See Also:
stepper: galileo+


Attachments
Draft v1 - Not complete (306.56 KB, patch)
2008-09-03 00:05 EDT, Simon Mc Duff CLA
no flags Details | Diff
Draft v2 (325.18 KB, patch)
2008-09-03 23:54 EDT, Simon Mc Duff CLA
no flags Details | Diff
Draft v3 (329.18 KB, patch)
2008-09-04 19:21 EDT, Simon Mc Duff CLA
no flags Details | Diff
Patch (reviewed by Eike) (336.42 KB, patch)
2008-09-05 08:10 EDT, Eike Stepper CLA
no flags Details | Diff
Draft v4 (with Eike review) (339.63 KB, patch)
2008-09-05 11:42 EDT, Simon Mc Duff CLA
no flags Details | Diff
Support EClass (348.38 KB, patch)
2008-09-07 20:28 EDT, Simon Mc Duff CLA
no flags Details | Diff
Alpha v1 - Merged with HEAD (399.17 KB, patch)
2008-09-09 23:19 EDT, Simon Mc Duff CLA
no flags Details | Diff
Added Testcases (399.56 KB, patch)
2008-09-10 08:12 EDT, Simon Mc Duff CLA
no flags Details | Diff
Alpha v2 (401.20 KB, patch)
2008-09-10 08:50 EDT, Simon Mc Duff CLA
no flags Details | Diff
Alpha v3 (403.31 KB, patch)
2008-09-10 10:20 EDT, Simon Mc Duff CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eike Stepper CLA 2007-12-19 00:33:21 EST
Simon McDuff schrieb:
>     "Eike Stepper" <stepper@sympedia.de> wrote in message news:fk91k7$g26$1@build.eclipse.org...
>     Simon McDuff schrieb:
>>     "Eike Stepper" <stepper@sympedia.de> wrote in message 
>>     news:fk8ses$vt0$4@build.eclipse.org...
>>       
>>>     Simon,
>>>
>>>     I think my main point is another one:
>>>     It relates a bit to the last sentence of you "where it will connect 
>>>     automatically to the right CDOSEssion".
>>>     Can you please explain how you plan to persist enough information for the 
>>>     proxy that an arbitrary client can reconsrtuct that right session?
>>>         
>>     I will use URI of the object. In the URI it contains : Resource + object 
>>     identifier.
>>
>>     So when you will try to load that objects from its URI.. it will go the 
>>     resourset.. and load it from there.
>>     You will have register your resource in the resourset. SO the specific 
>>     resource could be assocaite with any CDOTransaction.
>>       
>     Currently in CDO there is a 1:1 association between resource set and transaction. Would you like to change that?
>     Yes, It would be more a resource attach to a transaction. DO you see any problems ?
>
I do see a lot of technical issues although.that alone doesn't prevent from an eventual solution ;-)
I do see a major shift of paradigm as well.

You know that I'm generally not against technical challenges and interesting new features but I still believe that this undertaking would have a major impact on the existing concepts and solutions. This has at least two consequences for me (and you):

1) Before digging into code of an eventual patch (or even creating such a patch) I'd rather like to see something like a feasibility studie with a detailed spec of the problem, a detailed spec of the suggested solution and a detailed impact analysis. I know that thos would much easier if there was a detailed spec of the current solution. But your previous feature requests (btw. with cool results!) and the expectable issues with the large code base could be blamed for the minor progress I'm achieving with the documentation ;-) May be this is the point where I'm finally pushed to write a detailed technical spec of the whole CDO system...

2) This change could not be published as a normal I-build. It would force the creation of a new branch. Absolutely doable but a lot of effort, too. If you want to start prototyping I strongly recommend that you do that in a separate workspace (your local branch).

>
>>>     But even more important your scenario always predicts that all clients go 
>>>     through the same set of repositories as members of the XA. Would you vote 
>>>     for denying access to a single repository only because it was used in a XA 
>>>     commit?
>>>         
>>     I don't understand your question. Can you clarify your thoughts ? I will try 
>>     to answer.. but not sure if it will answer your question.
>>     XA point of view is .. It doesn't matter if objects are from the same repo 
>>     or different repo. It should always work.... in both cases.
>>     My scenario predicts that I will go the repositories that was register in 
>>     the resourceset. At this point it doesn't know if it is a XA transaction.
>>     We will add XA to ensure the integrity of the graph.
>>       
>     I'll try to say it with an example (given we had inter-repo refs):
>
>     - Client 1 uses a distributed transaction to commit into 2 repositories repoA and repoB. An inter-repo ref [R] from repoA -> repoB is part of the transaction.
>     - Some time later client 2 uses a normal transaction/view to navigate the object graph of repoA. When it tries to dereference [R] it comes to an InconsistentObjectGraphException.
>      
>     Yes we had no choice.
>     It should react exactly the same way as we were doing crossreference between resources in EMF.. The resource XXX isn't reachable..
>     Right ?
>
No, this is clearly not according to the current requirements (settled years ago and stabilized through lots of experience with CDO). But before we start to disput on selected technical details I'd really suggest that we (mainly you) start to describe the whole solution.

>>>
>>>     Simon McDuff schrieb:
>>>         
>>>>     Aside from the XA discussion what exactly do you mean with "adjust 
>>>>     references"?
>>>>
>>>>     Let say I have objectA(from repoA) and objectB(From repoB)
>>>>     objectA.setXXX(objectB);
>>>>     objectB.setXXX(objectA);
>>>>     Both of them need to know the id of the other objects. Important: id plus 
>>>>     session!
>>>>
>>>>
>>>>     They will need to adjust their reference after sending the request. I 
>>>>     still don't know what you mean ;-(
>>>>     [SIMON]Both repo aren't necessarly  in the same server. Could be at two 
>>>>     differences places.
>>>>
>>>>     I'd say you were right if it was feasible *and* appropriate.
>>>>     Up to now I argued that it's not feasible (see above) but I'd appreciate 
>>>>     to discuss the appropriateness if you can proof that it's feasible at 
>>>>     all.
>>>>
>>>>     Repos will only need to be able to save PROXY.(CDOIDTypeImpl).
>>>>     After that it is easy!!
>>>>     - You have XA to ensure the integrity of the graph. - Each repos could 
>>>>     save PROXY(URI)
>>>>     - JPA will do the mapping between them.
>>>>          - Each resource will be associate with a repo. This mapping will  be 
>>>>     done through JPA.
>>>>     - When you load an object, to resolve it it will go through the 
>>>>     resourceset.. where it will connect automatically to the right CDOSEssion 
>>>>     to get to the data.
>>>>
>>>>     I don't see where it is not feasible.
Comment 1 Eike Stepper CLA 2008-06-10 02:30:43 EDT
Reversioned due to graduation
Comment 2 Simon Mc Duff CLA 2008-07-24 22:42:18 EDT
Eike and others, I would like to start to work on this bugs if no one already start it. 

As discussed, I expect to change the way we link resourceset , resource, view... don`t know yet but I will think about it.

The solution will allow the same resourceset to have :
- Many resources that aren't from the same CDOView.(or/and CDOSession)
- Objects from one resource can refer objects in another resource even if they belong to another repository.
- Objects from one resource can refer objects in another resource even if they do not belong to any repository.(e.g.: XMIResource)

(this imply that objects from one resource can refer objects in another resource even if they belong or not to a CDOView).

The only change IStore implementor would need to change is to support that an object could refer to a
CDOID or a Proxy (something like that). 
Should all IStore  support that feature ? Should we add a support options ?

Let me know if we have others requirements.
Comment 3 Victor Roldan Betancort CLA 2008-07-25 05:08:21 EDT
Hi Simon,

I've some comments on this bug, since I discussed it with Eike in the newsgroup.

The requirements you present are indeed very nice, specially the abstraction of type of resource (CDOResource, XMIResource,...). This would make it even possible to reference non-CDO enabled metamodels outside of the repository. Will all this, and as Eike pointed in the newsgroup, it would be good to broaden the bug name to something like "Support external references".

This bug might need also improvements in the import wizards. I was going to raise a bug to allow several dependant XMIResources to be imported. Those could be scattered within resources in a repository, or even within different repositories. 

Also the "load resource..." action in the CDOView editor should be improved to be able to load resources from other repositories.

I'm willing to help out with this enhancement.
Comment 4 Simon Mc Duff CLA 2008-07-25 12:02:00 EDT
Good suggestions Victor!!

I see the improvement of the import wizard and the Load resource as new bugs entry.

Feel free to raise them!
Comment 5 Simon Mc Duff CLA 2008-08-01 13:18:56 EDT
Question for Eike,

I didn`t start to code yet but I was just thinking about how I will do the ResourceSet things and the PackageRegistry. I have many solutions.

My question is more about the current system.
Since we can register package to a CDOSession and we have the feature DemandPopulating for the PackageRegistry, shouldn`t want to use always these features ? This mean setSelfPopulatingPackageRegistry and the default CDOPackageRegistry are useless.

I don`t know in which case we will not want to use them ?

In my new scenario, I want to
- Be able to register EPackage directly from the CDOSession. (Like before)
- Register EPackage from the resourceSet should not impact packageRegistry in CDOSession. Many EPackage could be use without being use in a specific Resource.
- It should ALWAYS register automatically EPackage in CDOSession when 1) objects are persisted and their classes are not registered.

This will imply that we should remove the following:

  public void setSelfPopulatingPackageRegistry()
  {
    setPackageRegistry(CDOUtil.createSelfPopulatingPackageRegistry());
  }

  public void setDemandPopulatingPackageRegistry()
  {
    setPackageRegistry(CDOUtil.createDemandPopulatingPackageRegistry());
  }

and always use the setDemandPopulatingPackageRegistry. (Maybe put it more in the core)

Can you refresh my mind  why we shouldn`t want to use setDemandPopulatingPackageRegistry feature ?

I could keep the way it was but I was wondering why we gave options to the customers.... why not always register EPackage that are not there.

Simon
Comment 6 Simon Mc Duff CLA 2008-08-01 13:24:37 EDT
OOps.. I just understood something. Forget my last comments. I will re-phrase it in a few minutes!
Comment 7 Simon Mc Duff CLA 2008-08-01 13:42:02 EDT
In my new scenario, I will 
- Be able to register EPackage directly from the CDOSession. (Like before)
- Register EPackage from the resourceSet should not impact packageRegistry in CDOSession. Many EPackage could be use without being use in a specific Resource. 

Because of that 
- One general PackageRegistry attached from the ResourceSet 
- Each CDOSession would have an independent PackageRegistry. 
- The general PackageRegistry will contain all Epackage that are from every CDOSession active (that are part from the ResourceSet).
- Each PackageRegistry from every CDOSession active will not have have all EPackage that the general PackageRegistry have but only the one that the user registered or the one that are needed.

It will not be true anymore saying that ResourceSet is attach to only one CDOView.


Comment 8 Eike Stepper CLA 2008-08-01 13:46:56 EDT
(In reply to comment #5)
> My question is more about the current system.
> Since we can register package to a CDOSession and we have the feature
> DemandPopulating for the PackageRegistry, shouldn`t want to use always these
> features ? This mean setSelfPopulatingPackageRegistry and the default
> CDOPackageRegistry are useless.

I don't think so. Maybe in some core-dependent application but an editor would have to know from which packages to offer classes to instantiate *before* an instance of such class is already added to a resource.
Comment 9 Eike Stepper CLA 2008-08-01 13:56:30 EDT
(In reply to comment #7)
> In my new scenario, I will 
> - Be able to register EPackage directly from the CDOSession. (Like before)
> - Register EPackage from the resourceSet should not impact packageRegistry in
> CDOSession. Many EPackage could be use without being use in a specific
> Resource.
 
These "two" package registries are the *same*.
--> org.eclipse.emf.internal.cdo.CDOSessionImpl.attach(ResourceSet, CDOViewImpl)


> Because of that 
> - One general PackageRegistry attached from the ResourceSet 

What do you mean here by "attached from the ResourceSet"?


> - Each CDOSession would have an independent PackageRegistry. 

Like before.


> - The general PackageRegistry will contain all Epackage that are from every
> CDOSession active (that are part from the ResourceSet).
> - Each PackageRegistry from every CDOSession active will not have have all
> EPackage that the general PackageRegistry have but only the one that the user
> registered or the one that are needed.

It's still not completely clear to me what you mean.


> It will not be true anymore saying that ResourceSet is attach to only one
> CDOView.

I still believe that the devil is sitting in the implementation details but I'm looking forward to see a solution!
Comment 10 Simon Mc Duff CLA 2008-08-01 14:04:40 EDT
> These "two" package registries are the *same*.
> org.eclipse.emf.internal.cdo.CDOSessionImpl.attach(ResourceSet, CDOViewImpl)

I know that but in my new Implementation they will be different.

> I still believe that the devil is sitting in the implementation details but I'm looking forward to see a solution!
AHAH!!

Comment 11 Simon Mc Duff CLA 2008-08-02 09:09:52 EDT
EIke,

Is it a requirements that we do not create our own ResourceSet implementation ? Or it is something that we can do ?

Simon
Comment 12 Simon Mc Duff CLA 2008-08-02 11:39:56 EDT
OK I started to implement this feature.
It took more time to think about it to do the first phase.

The first step is to be able to use multiple Resource from different source (CDOView, XMIResource etc.) at the same time.

In the case where the following is called:

resourceset.createResource(...);

I need to decide in which CDOTransaction I will create this resource. (only if it start from cdo:/)
It is why I use the following concept :

CDOView.useAsDefaultDelegate(); // the name is not final yet.
for the getResource.. I will ask each of them.

In one of my testcase I can do the following :

      Supplier supplierD = (Supplier)resD.getContents().get(1);
      PurchaseOrder purchaseOrder = supplierD.getPurchaseOrders().get(0);

resD and supplierD are from XMIResourceImpl
purchaseOrder are from CDOView from another resource. Both resource belong to the same resourceSet. I can do that since XMIResourceImpl support external reference. The other way around isn't possible for now since CDO doesn't support it "yet".

The next step will be to refactor the code I`ve made... and to be able to support external reference from CDO Repository. 

I will need to implement 2 phases commit since the objectID (for new object) that come from CDO will change and other context need to have the right CDOID. The second phase will be use to exchange new CDOID between multiple CDOTransactions.

Also, I will include a feature that commit all CDOTransactions that are dependent to each to keep the integrity of the graph.
By default, it will be on but could be turned off. In that case, you could end-up with exceptions when you commit.

Comments are welcome.


Comment 13 Eike Stepper CLA 2008-08-02 13:23:18 EDT
(In reply to comment #11)
> Is it a requirements that we do not create our own ResourceSet implementation ?
> Or it is something that we can do ?

A resourceset-type-agnostic approach is preferrable for some reasons. Took me a while to google this old thread: http://www.eclispezone.com/eclipse/forums/t58476.html (search for "deriving a class from ResourceSet is discouraged").
Comment 14 Eike Stepper CLA 2008-08-02 13:37:55 EDT
If a ResourceSet is no longer associated with a single CDOSession we need to embed enough information into *each* CDOObject URI so that the ResourceSet (maybe via URIConverter) is able to calculate the following function:

       CDOView view = getView(uri);

What if the client is responsible for registering the appropriate view-providers and can decide about the URI format?
Comment 15 Simon Mc Duff CLA 2008-08-02 15:05:46 EDT
(In reply to comment #14)
> If a ResourceSet is no longer associated with a single CDOSession we need to
> embed enough information into *each* CDOObject URI so that the ResourceSet
> (maybe via URIConverter) is able to calculate the following function:
>        CDOView view = getView(uri);
> What if the client is responsible for registering the appropriate
> view-providers and can decide about the URI format?

It will only be useful for 
resourceset.createResource(...);

An object need to have the same uri independently which CDOView they belong. With that in minds I wonder how we will CDOView view = getView(uri). 

I don`t see how we are going to do that.

I prefer to use a default view. But when we do the following:

getEObject(URI);

It will go through the list of view until it can locate the first resource that match the URI. After that, the resource is registered to the resourceset.

Now I can use external object from CDO to XMI and XMI to CDO.
The next step is from CDO to CDO. For that I need the two phase commit.

Comment 16 Simon Mc Duff CLA 2008-08-02 23:49:27 EDT
I would like your comments on the following:

Do you see any problems having at commit time one thread per CDOTransaction involve in the XATransaction (kind of) ? 
I don`t see having thousand of CDOTransaction in the same resourceSet!! :-)

I could revisit it later or now, but so far my first draft have this limitation.

Comment 17 Eike Stepper CLA 2008-08-03 01:32:36 EDT
Simon, if you're already changing code please do so in a separate branch (not your combined mass branch).

To your question: I think we can generally postulate the limitation that a CDOView is not thread-safe with respect to multiple clients. As a consequence we can safely assume that at any time only one thread accesses a view/transaction. The only remaining issue to consider are listener callbacks from the session which could stem from server initiated requests like invalidation. These occur in the scope of their own indication treads.
Comment 18 Simon Mc Duff CLA 2008-08-03 09:51:51 EDT
Hi Eike,
I used my combined mass branch but without the intention of committing the code. Before we merged (mass branch), I will create the patch and continue in HEAD. I did that because I need to modify the commit process and wanted to have the latest code.

CDOView is not thread-safe ? I thought with all the modifications we've made to synchronize  CDOViewImpl (like.getObject) that CDOView was thread safe.
I remember that we had problem (with CDO callback) and because of that we needed to put CDOView more thread-safe. 
I know we used CDOView at work in a multi-threaded.. and worked fine. It was necessary since we never know when the UI will access objects and we had threads that was doing some works as well. (client side)

I asked you that question about if CDOTransaction.commit() because when we commit one CDOTransaction (and the feature is turned on) it will commit at the same time all CDOTransaction that are included in a joinTransaction (XATransaction concept). We do that to keep the integrity of the graph and to not bother knowing which transaction should we commit first. Also in some cases it it necessary. 
e.g.: Objects A->Object B and ObjectA <- ObjectB 
Object A and Object B are new
Object A and Object B belong to two differents CDOView.(also repo)

Since our commit process doesn't allow us to take back control at specific point in the code, I need to change that and do it by step. In the future, we will even be able to use the following:
https://bugs.eclipse.org/bugs/show_bug.cgi?id=213403 [XA] Support distributed transactions

I've created a CDOIDProxyImpl CDOIDProxyTempImpl. 
CDOIDProxyImpl isProxy() == true isTemp == false. (External object that their URN will not changed, e.g.: objects that belong to an XMIResource)
CDOIDProxyTempImpl isProxy() == true isTemp == true.(External object that their URN are temporary, it needs to exchange at commit time. e.g.: new objects that belong to an CDOResource)

I would like it to derive from CDOIDTemp to be able to use it in the mapping when we commit.

I was wondering if CDOIDTemp needs to have the following:
public int getIntValue();

We use it only at one place...probably I will remove it or/and add an interface.

I think the most place I need to do modification is in the commit process... the other modification was minor.

So far I'm really happy that we didn't encounter really big issue. The commit process needs to be well define and after that it should be alright!
Comment 19 Eike Stepper CLA 2008-08-03 10:19:36 EDT
(In reply to comment #18)
> I asked you that question about if CDOTransaction.commit() because when we
> commit one CDOTransaction (and the feature is turned on) it will commit at the
> same time all CDOTransaction that are included in a joinTransaction
> (XATransaction concept). We do that to keep the integrity of the graph and to
> not bother knowing which transaction should we commit first. 

Basically I'm fine with the threading thing. But I don't like a transaction being implicitely committed due to another commit. Maybe we should change the semantics of our transaction completely. I.e. better separate the concepts of views and transactions. The client must always first fetch a CDOView and if it wants to modify something it must associate the CDOView with a CDOTransaction. Then it depends on the type of the transaction (XA or not) what happens... CDOTransaction would no longer inherit from CDOView.

Btw. then I'd also like to factor the CDOAudit aspects into a more general CDOViewDiscriminator with implementations like LATEST and HISTORICAL.

> Also in some cases
> it it necessary.
> e.g.: Objects A->Object B and ObjectA <- ObjectB
> Object A and Object B are new
> Object A and Object B belong to two differents CDOView.(also repo)
> 
> Since our commit process doesn't allow us to take back control at specific point
> in the code, I need to change that and do it by step. 

No clue what you mean here ;-(

> In the future, we will
> even be able to use the following:
> https://bugs.eclipse.org/bugs/show_bug.cgi?id=213403 [XA] Support distributed
> transactions
> 
> I've created a CDOIDProxyImpl CDOIDProxyTempImpl.
> CDOIDProxyImpl isProxy() == true isTemp == false. (External object that their
> URN will not changed, e.g.: objects that belong to an XMIResource)
> CDOIDProxyTempImpl isProxy() == true isTemp == true.(External object that their
> URN are temporary, it needs to exchange at commit time. e.g.: new objects that
> belong to an CDOResource)
> 
> I would like it to derive from CDOIDTemp to be able to use it in the mapping
> when we commit.
> 
> I was wondering if CDOIDTemp needs to have the following:
> public int getIntValue();

What would be your alternative?
Comment 20 Simon Mc Duff CLA 2008-08-03 10:51:56 EDT
> Basically I'm fine with the threading thing. But I don't like a transaction
> being implicitely committed due to another commit. 
> semantics of our transaction completely. I.e. better separate the concepts of
> views and transactions. The client must always first fetch a CDOView and if it
> wants to modify something it must associate the CDOView with a CDOTransaction.
> Then it depends on the type of the transaction (XA or not) what happens...
> CDOTransaction would no longer inherit from CDOView.
In fact, I was doing the same things except without touching anything about CDOTransaction and CDOView.
I had an object that is a CDOXATransaction. 
You can join it like the following:
CDOTransaction.joinTransaction(CDOXATransaction);

at the end you could do the following

CDOXATransaction.commit()
or
CDOTransaction.commit(); // When CDOTransaction is in a XATransaction it delegate to its CDOXATransaction.

CDOTransactionImpl like it is right now will always be used... (XATransaction or not) because it contains all the code for changes.
CDOXATransaction is a matter of using a list of CDOTransaction and being able to call:
CDOTransaction.commit_phase1()
CDOTransaction.commit_phase2()
CDOTransaction.commit_phase3()


CDOTransactionImpl should still inherit from CDOViewImpl but maybe just the Transaction management (commit rollback savepoint) should be redefine.


> 
> Btw. then I'd also like to factor the CDOAudit aspects into a more general
> CDOViewDiscriminator with implementations like LATEST and HISTORICAL.

> 
> > Also in some cases
> > it it necessary.
> > e.g.: Objects A->Object B and ObjectA <- ObjectB
> > Object A and Object B are new
> > Object A and Object B belong to two differents CDOView.(also repo)
> >
> > Since our commit process doesn't allow us to take back control at specific
> point
> > in the code, I need to change that and do it by step.

> No clue what you mean here ;-(
Here some explanation: (go back to my example)
CDOID of objecta would be CDOIDTemp - 1
CDOID of objectb would be CDOIDTemp - 100
Their URN is based of the CDOID.
if  Objects A->Object B and ObjectA <- ObjectB
This mean that when I will commit let say transactionA it will persist objectA. At this point objectA think that objectB CDOID is 100.
After committing transactionA, we will commit transactionB. After committing transactionB, objectB will change CDOID to whatever the store persist it.(it is not temporary anymore)
When we will try to fetch objectB from objectA we will try to load CDOIDTemp 100... and this one doesn't exist anymore. It is why they need to exchange value at this time.

> 
> > In the future, we will
> > even be able to use the following:
> > https://bugs.eclipse.org/bugs/show_bug.cgi?id=213403 [XA] Support distributed
> > transactions
> >
> > I've created a CDOIDProxyImpl CDOIDProxyTempImpl.
> > CDOIDProxyImpl isProxy() == true isTemp == false. (External object that their
> > URN will not changed, e.g.: objects that belong to an XMIResource)
> > CDOIDProxyTempImpl isProxy() == true isTemp == true.(External object that
> their
> > URN are temporary, it needs to exchange at commit time. e.g.: new objects that
> > belong to an CDOResource)
> >
> > I would like it to derive from CDOIDTemp to be able to use it in the mapping
> > when we commit.
> >
> > I was wondering if CDOIDTemp needs to have the following:
> > public int getIntValue();
> 
> What would be your alternative?
Comment 21 Simon Mc Duff CLA 2008-08-13 19:40:29 EDT
The first draft is done. It is working very well.
By default, I think auto-commit feature (commit all CDOtransaction at the same time that have the same Resourceset) should be turned on by default since your graph will keep the integrity. If you are using only one CDOTransaction, it should not have any overhead!

I still need to make :
- my code more robust
- Add exception with good message
- Refactor the name of my new classes 
- Simplify my code
- implement rollback, setSavePoint( Should be easier since only client side code is affected).
- Add more testcases.
- code review with Eike

Let me know if you have any comments.

Comment 22 Simon Mc Duff CLA 2008-08-13 19:50:12 EDT
Eike

also I changed many things but something I would like to talk to you in advance.

IStoreWriter

  /**
   * Could be called many times before commit().
   * @since 2.0
   */
  public void write(CommitContext context);
  
  /**
   * It will flush to disk and make data available for others.
   * @since 2.0
   */
  public void commit();
  
  I need to distinct between writing.. and commit to disk. I needed to differentiate and control what I write ... and when we make available data to others.
  
  What do you think ?
Comment 23 Simon Mc Duff CLA 2008-08-14 14:16:44 EDT
Eike and I decided the following restrictions:

The URI will change for the following:
  cdo://<repo-uuid>/<resource-path>#<cdoid>

Also only one transaction with the same repo should be allowed for the same ResourceSet.
  ResourceSet---> Map<<repo-uuid>, <CDOView>>

  We should use <repo-uuid> to know in which view to create a resource and how to retrieve CDOResource.
  
  
  
Comment 24 Simon Mc Duff CLA 2008-08-14 22:53:42 EDT
Just to let you know that 

createResource("/resA");
createResource("resA");

Theirs URIs are exactly the same things:
cdo:/<REPOS-UUID>/resA

We could or not add the separator("/") at the beginning.

Let me know if you see a problem there.
Today I did the following:

- I changed the UUID. I will need to change many testcases !! :-)
- CDOResourceFactoryImpl isn't a singleton anymore. You have one by CDOViewSet.
  CDOViewSet.getResourceFactory()

Now I will concentrate on the PackageRegistry. 
ExternalReference are working again with the 3 phases commit. 
I need to refactor for a few days....once it is done I will be able to show it to you.

Sorry I had some visit at home today and for the next day.. so I will not be as productive !! :-) 



Comment 25 Eike Stepper CLA 2008-08-15 03:17:46 EDT
I guess you mean createResource() of CDOTransaction, right? Your new implementation?

I think if the path has a prefix slash or not does not matter if we declare it as always being absolute.
But the resulting URI should start with "cdo://" (double slash). AFAIK that means a hierarchical URI with an authority (==repo).

I always have to write a small test prog to be sure again about these URI parts, something like:

  public static void main(String[] args)
  {
    dump(URI.createURI("cdo:///a/b/c"));
    dump(URI.createURI("cdo://a/b/c"));
    dump(URI.createURI("cdo:/a/b/c"));
    dump(URI.createURI("cdo:a/b/c"));

    URI uri = URI.createURI("cdo://repo");
    uri.appendSegment("/res");
  }

  private static void dump(URI uri)
  {
    System.out.println(uri);
    System.out.println();
    System.out.println("    isHierarchical: " + uri.isHierarchical());
    System.out.println("    isPrefix:       " + uri.isPrefix());
    System.out.println("    isRelative:     " + uri.isRelative());
    System.out.println("    authority:      " + uri.authority());
    System.out.println("    path:           " + uri.path());
    System.out.println("    devicePath:     " + uri.devicePath());
    System.out.println("    segmentsList:   " + uri.segmentsList());
    System.out.println("    opaquePart:     " + uri.opaquePart());
    System.out.println();
  }
  
This prints:

cdo:///a/b/c

    isHierarchical: true
    isPrefix:       false
    isRelative:     false
    authority:      
    path:           /a/b/c
    devicePath:     ///a/b/c
    segmentsList:   [a, b, c]
    opaquePart:     null

cdo://a/b/c

    isHierarchical: true
    isPrefix:       false
    isRelative:     false
    authority:      a
    path:           /b/c
    devicePath:     //a/b/c
    segmentsList:   [b, c]
    opaquePart:     null

cdo:/a/b/c

    isHierarchical: true
    isPrefix:       false
    isRelative:     false
    authority:      null
    path:           /a/b/c
    devicePath:     /a/b/c
    segmentsList:   [a, b, c]
    opaquePart:     null

cdo:a/b/c

    isHierarchical: false
    isPrefix:       false
    isRelative:     false
    authority:      null
    path:           null
    devicePath:     null
    segmentsList:   []
    opaquePart:     a/b/c

Exception in thread "main" java.lang.IllegalArgumentException: invalid segment: /res
	at org.eclipse.emf.common.util.URI.appendSegment(URI.java:2497)

---
What do you mean by "changed the UUID"?

---
Of course CDOResourceFactoryImpl does not need to be a singleton ;-)

---
The main challenge with multi-delegating package registries is to meet the intended contract, btw. find what the contract is (EMF does not fulfil the Map contract!).

---
I can't believe that some visit prevents you from being productive!!! ;-P
Comment 26 Simon Mc Duff CLA 2008-08-15 12:47:31 EDT
(In reply to comment #25)
> I guess you mean createResource() of CDOTransaction, right? Your new
> implementation?
No, I mean CDOResourceFactory. CDOTransaction.createResource delegate to the factory.
> 
> I think if the path has a prefix slash or not does not matter if we declare it
> as always being absolute.
> But the resulting URI should start with "cdo://" (double slash). AFAIK that
> means a hierarchical URI with an authority (==repo).

I can see a problem using createResource("/resA"). Here the result :
"cdo:/<repos>//resA" (3 segments<repos>, blank, resA)

I think the user mean something more like 
"cdo:/<repos>/resA" (2 segments)

We could start it with "//" but the user will have no control about that because we now put repo between... By the way, why should it start with "\\" ?.
> 
> I always have to write a small test prog to be sure again about these URI parts,
> something like:
> 
> public static void main(String[] args)
> {
> dump(URI.createURI("cdo:///a/b/c"));
> dump(URI.createURI("cdo://a/b/c"));
> dump(URI.createURI("cdo:/a/b/c"));
> dump(URI.createURI("cdo:a/b/c"));
> 
> URI uri = URI.createURI("cdo://repo");
> uri.appendSegment("/res");
> }
> 
> private static void dump(URI uri)
> {
> System.out.println(uri);
> System.out.println();
> System.out.println("    isHierarchical: " + uri.isHierarchical());
> System.out.println("    isPrefix:       " + uri.isPrefix());
> System.out.println("    isRelative:     " + uri.isRelative());
> System.out.println("    authority:      " + uri.authority());
> System.out.println("    path:           " + uri.path());
> System.out.println("    devicePath:     " + uri.devicePath());
> System.out.println("    segmentsList:   " + uri.segmentsList());
> System.out.println("    opaquePart:     " + uri.opaquePart());
> System.out.println();
> }
> 
> This prints:
> 
> cdo:///a/b/c
> 
> isHierarchical: true
> isPrefix:       false
> isRelative:     false
> authority:
> path:           /a/b/c
> devicePath:     ///a/b/c
> segmentsList:   [a, b, c]
> opaquePart:     null
> 
> cdo://a/b/c
> 
> isHierarchical: true
> isPrefix:       false
> isRelative:     false
> authority:      a
> path:           /b/c
> devicePath:     //a/b/c
> segmentsList:   [b, c]
> opaquePart:     null
> 
> cdo:/a/b/c
> 
> isHierarchical: true
> isPrefix:       false
> isRelative:     false
> authority:      null
> path:           /a/b/c
> devicePath:     /a/b/c
> segmentsList:   [a, b, c]
> opaquePart:     null
> 
> cdo:a/b/c
> 
> isHierarchical: false
> isPrefix:       false
> isRelative:     false
> authority:      null
> path:           null
> devicePath:     null
> segmentsList:   []
> opaquePart:     a/b/c
> 
> Exception in thread "main" java.lang.IllegalArgumentException: invalid segment:
> /res
> at org.eclipse.emf.common.util.URI.appendSegment(URI.java:2497)
> 
> ---
> What do you mean by "changed the UUID"?
Sorry changed URI :-)
> 
> ---
> Of course CDOResourceFactoryImpl does not need to be a singleton ;-)
> 
> ---
> The main challenge with multi-delegating package registries is to meet the
> intended contract, btw. find what the contract is (EMF does not fulfil the Map
> contract!).
> 
> ---
> I can't believe that some visit prevents you from being productive!!! ;-P
:-)
Comment 27 Eike Stepper CLA 2008-08-15 12:55:07 EDT
(In reply to comment #26)
> > I guess you mean createResource() of CDOTransaction, right? Your new
> > implementation?
> No, I mean CDOResourceFactory. CDOTransaction.createResource delegate to the
> factory.

Ah, I see!

> > I think if the path has a prefix slash or not does not matter if we declare it
> > as always being absolute.
> > But the resulting URI should start with "cdo://" (double slash). AFAIK that
> > means a hierarchical URI with an authority (==repo).
> 
> I can see a problem using createResource("/resA"). Here the result :
> "cdo:/<repos>//resA" (3 segments<repos>, blank, resA)

Yes, cdo:/<repos>//resA is bad. We should allow the path to be passed with or without prefix slash. Inside the method we remove all prefix slashes and construct the URI something like  ...reposUUID + "/" + path. Ok?

> I think the user mean something more like 
> "cdo:/<repos>/resA" (2 segments)
> 
> We could start it with "//" but the user will have no control about that
> because we now put repo between... By the way, why should it start with "\\" ?.

Who talked about backslashes? I meant two slashes instead of only one after the scheme. that turns the first segment of the path into the URI authority which makes sense for the repo.
Comment 28 Simon Mc Duff CLA 2008-08-15 13:28:20 EDT
> Yes, cdo:/<repos>//resA is bad. We should allow the path to be passed with or
> without prefix slash. Inside the method we remove all prefix slashes and
> construct the URI something like  ...reposUUID + "/" + path. Ok?
Back to my first comments:

Just to let you know that 
createResource("/resA");
createResource("resA");

Theirs URIs are exactly the same things:
cdo://<REPOS-UUID>/resA

If I understand correctly what I did is correct ? (I just added "//")
Comment 29 Eike Stepper CLA 2008-08-15 13:29:05 EDT
Great!
Comment 30 Simon Mc Duff CLA 2008-08-15 23:20:28 EDT
I know that UUID could be very bothering if you want to look manually objects ? 
(By the way UUID is only a string.. so we can put the same string as the repository name... right ? )

Should we be able to support the old format : cdo:/test1 
Only for the following (for input)
- Create Resources
- Retrieve objects
- CDOViewSet must have one CDOView, otherwise we will throw an exception.
- We will always return the reposUUID in the URI.

Make senses, if you don't have the authority and you provided only one CDOView... we will use that one.

That way, we will support the old fashion.
    
The code that is affected by that is only in the CDOView, ProxyResolverURIResourceMap and CDOResourceFactory. (I think)
 
 What do you think ?
Comment 31 Eike Stepper CLA 2008-08-16 02:38:09 EDT
(In reply to comment #30)
> I know that UUID could be very bothering if you want to look manually objects ? 
> (By the way UUID is only a string.. so we can put the same string as the
> repository name... right ? )

Hmm, what about automatically converting repo-names into repo-uuids if possible?
We could call the authority part of the URI repo-id and try to resolve via repo-uuid first and repo-name second...

> Should we be able to support the old format : cdo:/test1 
> Only for the following (for input)
> - Create Resources
> - Retrieve objects
> - CDOViewSet must have one CDOView, otherwise we will throw an exception.
> - We will always return the reposUUID in the URI.
> 
> Make senses, if you don't have the authority and you provided only one
> CDOView... we will use that one.
> 
> That way, we will support the old fashion.

That's a good idea!

> The code that is affected by that is only in the CDOView,
> ProxyResolverURIResourceMap and CDOResourceFactory. (I think)

ProxyResolverURIResourceMap is only for legacy support, but if it's obvious to adjust, we should do it.
Comment 32 Simon Mc Duff CLA 2008-08-16 16:13:15 EDT
Before we were able to do the following: (without any CDOView opened)
    final URI uri = URI.createURI("cdo:/test1");
    ResourceSet resourceSet = new ResourceSetImpl();
    CDOUtil.prepareResourceSet(resourceSet);
    CDOResource resource = (CDOResource)resourceSet.createResource(uri);

Now it does't make sense since the protocol CDO it is not available.

Can you confirm me that it should now throw an exception ?
Does it have an impact on the current way people are using it (in the editor)

 

Comment 33 Eike Stepper CLA 2008-08-17 01:40:59 EDT
(In reply to comment #32)
> Before we were able to do the following: (without any CDOView opened)
>     final URI uri = URI.createURI("cdo:/test1");
>     ResourceSet resourceSet = new ResourceSetImpl();
>     CDOUtil.prepareResourceSet(resourceSet);
>     CDOResource resource = (CDOResource)resourceSet.createResource(uri);
> 
> Now it does't make sense since the protocol CDO it is not available.

Hmm, I even don't think that it made sense before. A cdo: URI indicates the existance of a CDOView (now a CDOViewSet). But I'm not sure about that. What happens to resources that become detached? Should they disappear from the ResourceSet? Should they change their URI?

> Can you confirm me that it should now throw an exception ?

If we can't find any meaningful semantics for this construct we can throw an exception.

> Does it have an impact on the current way people are using it (in the editor)

I can imagine that the whole ext-ref feature has an impact on the editor and other clients of the cdo core. All these places must be adjusted accordingly before code is committed. Hard to say *how* people are using it.
Comment 34 Simon Mc Duff CLA 2008-08-17 10:26:37 EDT
What do you think of the following:

We can do the following only if you have 0 or 1 CDOView active. (In the case where 1 CDOView is active it will attach in that view.. we already discuss that one)

    final URI uri = URI.createURI("cdo:/test1");
    ResourceSet resourceSet = new ResourceSetImpl();
    CDOUtil.prepareResourceSet(resourceSet);
    CDOResource resource = (CDOResource)resourceSet.createResource(uri);
    assertTransient(resource);

    msg("Opening session");
    CDOSession session = openModel1Session();
    msg("Opening transaction");
    CDOTransaction transaction = session.openTransaction(resourceSet);
When the first CDOTransaction is attached to the CDOViewSet... we attach all transient CDOResources to it.

I think it make sense. 
    
    


Comment 35 Simon Mc Duff CLA 2008-08-17 11:57:34 EDT
Now I'm at the stage where all previous testcases are working.
I added new ones and some of them are failing because detach wasn't implemented.
e.g.:
- Detach, we should remove object from cache CDOTransaction...(objects or resources)

So to combine our work will be good!

I fixes some issues we had with notification from Resourceset  : NOTIFY_ADDMANY and NOTIFY_REMOVEMANY.
They were not well implemented and I added testcases.

Comment 36 Simon Mc Duff CLA 2008-08-21 23:10:49 EDT
I will wait until 

204890: Implement detach
https://bugs.eclipse.org/bugs/show_bug.cgi?id=204890

is committed... since I moved some code that changed in another Component. (example: commit process, rollback process, etc..)

Maybe I will need also to wait for 

244800: Towards a new CDO URI format and handling
https://bugs.eclipse.org/bugs/show_bug.cgi?id=244800

Comment 37 Simon Mc Duff CLA 2008-09-02 23:54:02 EDT
After refactoring, I discover the following failed (before my modification)

    CDOResource resource = transaction.createResource("/test1");
    CDOResource resourceCopy = transaction.getOrCreateResource("/test1");
    assertEquals(resource, resourceCopy);
    
It will be corrected in that feature. It also improve performance for the following call >>CDOView.getResourceID<< since it will go look at the cache first.
Comment 38 Simon Mc Duff CLA 2008-09-03 00:05:02 EDT
Created attachment 111547 [details]
Draft v1 - Not complete

Not complete( Missing CDOTransactionSet.rollback and setSavePoint) 
JAVADOC

Design :
- CDOTransaction have a CDOTransactionStrategy for commit, rollback, setsavepoint
- CDOUserTransaction contains method to commit, rollback and setsavepoint.
- Remove CommitContext from Transaction
- Add CommitContext for CDOTransaction.createCommitContext()
- 3 phases commit with CDOTransactionSet
- CDOURIUtil
- CDOViewImpl.attach/Detach
- CDOViewSet send add/remove resources message to the appropriate CDOView.
- CDOView -> CDOViewSet
- CDOViewSet-> ResourceSet
- CDOViewSet ->* CDOView
- Resource URI has been changed.
- Object URI has been changed.
- Fixes CDOView.geObject support recursive calls.
- Fixes CDOTransaction.getOrCreateResource return the same resource if already created. (Testcases as well)

We can discuss it tomorrow if you have time (and if I have time) :-)

I will now spend some time to remove loadTransitionResource...
Comment 39 Eike Stepper CLA 2008-09-03 10:29:52 EDT
(In reply to comment #37)

Good point!
Comment 40 Simon Mc Duff CLA 2008-09-03 23:54:27 EDT
Created attachment 111646 [details]
Draft v2

Support rollback and SetSavepoint.
This feature is now fonctionnal.

Need to be done:
- Add javadoc 
- Add Trace
- Grammar correction for new.htm
- TestLogic testCase.
- LoadTransitionResource.. I want to remove it.
Comment 41 Simon Mc Duff CLA 2008-09-04 19:21:17 EDT
Created attachment 111745 [details]
Draft v3

Finally I'm ready for review

- Include XATransaction
- Detach objects and resources
- Support External references
- URI new format
- Fixes few issues. (Like getOrCreateResource being call multiple times)
- Introduce CDOViewSet, CDOXATransaction, CDOUserTransaction.

What is good, even if I changed many things, it is 100% compatible with the previous way of doing things. It is only the internal design that changed and I added a few interfaces.

Since I suspect it will changes a bit after Eike review, I will wait to complete javadoc.
Comment 42 Eike Stepper CLA 2008-09-05 08:10:27 EDT
Created attachment 111808 [details]
Patch (reviewed by Eike)

Comments/questions as per private email...
Comment 43 Simon Mc Duff CLA 2008-09-05 11:42:10 EDT
Created attachment 111832 [details]
Draft v4 (with Eike review)

Comments in external e-mail
Comment 44 Simon Mc Duff CLA 2008-09-07 20:28:14 EDT
Created attachment 111919 [details]
Support EClass
Comment 45 Simon Mc Duff CLA 2008-09-09 08:00:33 EDT

(In reply to comment #10)
> That bugs allow me to learn that containment proxy exist and that CDO didn't
> handle it correctly.

"not correctly" implies a sort of bug but that's not the case. 
Consciously CDO does *not support* this feature of EMF.

> Before I didn't know it was possible to have a contained object be in a
> separate resource theb the container. :-)
> 
> By knowing that and with the fact that we are now supporting external
> reference... it is kind worth it looking to support it.

What do we gain?
[Simon] We gain that we support another feature of EMF. And we improve our code by using EMF code to detach/attach/remove/add instead of our own code (like handleContainment). In fact, I override ebasicSetCOntainer(... notification). and eSetResource() with exactly the same code in EBasicObject... except that can detect if an object move from one view to another to not detach/attach objects for nothing.


> Containment proxies or not.. it allow me to look at the means of 
> CDOObject.resource
> CDOObject.eInternalCOntaier
> 
> It seems that we didn't handle eDirectResource correctly all the time as well.
> We missed the fact that when we load the object eDirectResource is invalid.

It's possible that we didn't handle it correctly, but what is the resulting
problem? What do *we* define as correct? eDirectResource is an EMF internal...

[SIMON] Yes eDirectResource is an internal.. but it is used by EMF internally to remove back-reference for resource. I think it will fail a few testcases I have in mind.

> I think will be worth it to used has EMF (support containment proxy)
> CDORevision.resourceID could be null. Right now it is always set.
> CDORevision.containerID 
> CDORevision.container
> 
> What do you think ?

As I said, I see no real value in this undertaking.
But if you decide to give it a try I'm not against it.
My fear is only that some of the more fragile relationships that do exist in
the CDO code will make it hard to touch this area...

[SIMON] Do you know some place already ? 
So far I touched
- CDOStatemachine
- CDOResourceImpl
- CDOObjectImpl

Maybe I will backtrack... but so far, isn't that bad. 

Comment 46 Simon Mc Duff CLA 2008-09-09 23:19:30 EDT
Created attachment 112156 [details]
Alpha v1 - Merged with HEAD

- Attach/Detach object and resources changed.
- Optimization in CDOStore
- Optimization in CDOStateMachine.attach.
- Finally Removed LoadResourceTransition
- We reached 200 testcases!!!
- Contains the following fixes for:
246619: Support addition/removal of many resources
https://bugs.eclipse.org/bugs/show_bug.cgi?id=246619

246620: Error occurs when getOrCreateResource is called
https://bugs.eclipse.org/bugs/show_bug.cgi?id=246620

246622: CDOStore.set doesn't affect variable correctly - Could cause memory retention
https://bugs.eclipse.org/bugs/show_bug.cgi?id=246622

246630: Shouldn't override hasProxies and use default behavior in CDOObjectImpl.
https://bugs.eclipse.org/bugs/show_bug.cgi?id=246630

246705: Support containment proxies

https://bugs.eclipse.org/bugs/show_bug.cgi?id=246705
Comment 47 Eike Stepper CLA 2008-09-10 03:14:11 EDT
Great!!! Skype me when you're available...
Comment 48 Simon Mc Duff CLA 2008-09-10 08:12:25 EDT
Created attachment 112190 [details]
Added Testcases
Comment 49 Simon Mc Duff CLA 2008-09-10 08:50:53 EDT
Created attachment 112192 [details]
Alpha v2

resourceSet.remove do not generates a detach.
resource.delete(null) generates a detach.
MOdified testcases to show that.
246844: Implement Resource.delete()
https://bugs.eclipse.org/bugs/show_bug.cgi?id=246844
Comment 50 Simon Mc Duff CLA 2008-09-10 10:20:28 EDT
Created attachment 112204 [details]
Alpha v3

QueryResources was committed.
Neede to merge my patch with HEAD to remove conflicts
Comment 51 Eike Stepper CLA 2008-09-10 12:51:16 EDT
Sorry ;-)
Comment 52 Simon Mc Duff CLA 2008-09-11 02:39:40 EDT
Committed to HEAD
Comment 53 Eike Stepper CLA 2008-09-16 14:07:29 EDT
Fix available in HEAD: 2.0.0.I200809110653.
Comment 54 Eike Stepper CLA 2008-09-26 14:40:35 EDT
Fix available in HEAD: 2.0.0.I200809260911.
Comment 55 Eike Stepper CLA 2009-06-27 11:54:55 EDT
Generally available.