Skip to content

Conversation

@alex-kulakov
Copy link
Contributor

@alex-kulakov alex-kulakov commented Oct 16, 2020

Instead of opening a session and then select StorageNode for it we select a node and then open session for the selected node. It allows asynchronous session opening to work fine.

Old method of storage node selection is marked [Obsolete] but works for synchronously opened sessions.

@alex-kulakov
Copy link
Contributor Author

I'm also going to detect whether old method is applied for the sessions with opened connection and throw an exception telling that this is not supported.

using (var session = domain.OpenSession()) {
session.SelectStorageNode(WellKnown.DefaultNodeId);
_ = domain.StorageNodeManager.AddNode(nodeConfiguration);
var selectedNode = domain.SelectStorageNode(WellKnown.DefaultNodeId);
Copy link
Contributor

Choose a reason for hiding this comment

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

For cases like this, we would introduce GetOrAddNode method (see below).

var storageNode = domain.StorageNodeManager.GetOrAddNode(nodeConfiguration);
using (var session = storageNode.OpenSession()) {
  ...
}

Copy link
Contributor Author

@alex-kulakov alex-kulakov Oct 19, 2020

Choose a reason for hiding this comment

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

Do you build nodes on demand and use them just after build like in the example you showed?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, we do. On application start, we don't have any nodes added. On the first request, we build a domain and then add the corresponding node. All subsequent requests either use the existing node or add a new one on the first request to that node.

Copy link
Contributor

Choose a reason for hiding this comment

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

The disadvantage of the suggested approach is that GetOrAddNode method takes node configuration as a parameter but the existing GetNode method gets just node id. I think we would exclude node id from config and pass it separately. So the sample would transform as follows.

var storageNode = domain.StorageNodeManager.GetOrAddNode(nodeId, nodeConfiguration);
using (var session = storageNode.OpenSession()) {
  ...
}
// AddNode method should also be changed for consistency.
_ = domain.StorageNodeManager.AddNode(nodeId, nodeConfiguration);

Copy link
Contributor Author

@alex-kulakov alex-kulakov Oct 21, 2020

Choose a reason for hiding this comment

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

NodeConfiguration is required to have nodeId anyway, except for default node. It validates before StorageNode build

Copy link
Contributor

Choose a reason for hiding this comment

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

But it is used only in the validation. The Validate method is called by the internal BuildNode method. In turn, the BuildNode method is called only from the AddNode.
As you can see we can easily transform all this chain.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's why I added extra check for nodeId to GetOrAdd. Take a look at new new commits.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we keep it this way, or even remove the GetOrAddNode method for now?
I'd suggest merging this PR to the 6.0 branch and to the master then.
Once the code is in the master I'd think about further improvements.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will keep StorageNodeManager API the same. GetOrAdd functionality can be implemented as an extension in your solution. Later we could integrate your implementation to DataObjects.Net

@alex-kulakov alex-kulakov merged commit dcbe44e into 6.0 Oct 27, 2020
@alex-kulakov alex-kulakov deleted the storage-node-selection branch October 27, 2020 11:07
@alex-kulakov alex-kulakov changed the title WIP: New StorageNode selection mechanism New session opening mechanism for StorageNodes Oct 27, 2020
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