Will the freelock session creation process cause inconsistency?

[mo-avatar]

We just have an "Insert session to local cache failed." issue, when session creation increase to an high amount. After read the code, We find it that the freelock session creation change in the code base may cause the problem.

It is great to have an free lock CreateSessionProcessor to improve performance.

But it seems that , it has an potential to have multiple session from multiple user or client to have a same sessionid.
As an optimistic approach, I didn't find a way which will prevent an existing sessionid which has been persisted into meta data be substitute with the same session id but with different user, graph_addr and client_ip, and creation time etc.

Create session failure on occasion may be not a big deal , but it seems have break the constraint that every session can only belong to one client at a time . I think may be there are many design based on the assumption.
@wey-gu @critical27

[wey-gu]

I think #5762 / cn-discussion was just a tradeoff/optimistic design with metaD performance on session creation, and that indeed will sacrifice consistency here.

@critical27 @mo-avatar I am thinking of

• option 0: [using nanosecond as session_id]
  or
• option 1: [adding hashing of user + timestamp]

would mitigate such issue by maintaining the same design trade-off/gaining some level of consistency(given that id gen was outside of AtomicOp)?

I'm not sure if it's silly(with obvious issues I am not aware of) but i think it could be a lazy but enough mitigation(option 0)?

[mo-avatar]

Yes, may be nanosecond second will help to mitigate the problem. So far the sessionid is treated as an uint64 type essentially in the thrift model。So several components will need to change, if we want to change to something else, it will lead to incompatibility issue in production。So it might need to be carefully designed to solve the problem completely.

[mo-avatar]

As a supplement, I think that if we want to implement some kind of session level cache in the future, this may lead to some unknown issue.

[wey-gu]

Or just do truncation(leave the right side of the nanoseconds) to fit in u64 to avoid heavy changes? or... move session object creation/id gen inside AtomicOp?

[critical27]

I'm not sure whether #5762 would cause the problem you mentioned:

We find it that the freelock session creation change in the code base may cause the problem.

Although it is lock-free, but the AtomicOp is actually executed in serialize.

But if a schema id occurs more than once, maybe we could modify the way we create session id.

[mo-avatar]

I think the AtomicOp may not help with duplicate session id. Because session id is generated at the very beginning, so different session can have the same session id when execute simultaneously.
Here is the code in GraphSessionManager.cpp GraphSessionManager::createSession function.

      auto findPtr = activeSessions_.find(sid);
      if (findPtr == activeSessions_.end()) {
        auto sessionPtr = ClientSession::create(std::move(session), metaClient_);
        sessionPtr->setClientAddress(clientIp, clientPort);
        sessionPtr->charge();
        auto ret = activeSessions_.emplace(sid, sessionPtr);
        if (!ret.second) {
          return Status::Error("Insert session to local cache failed.");
        }

We just find the error status "Insert session to local cache failed." has emerged, though at a very low frequency. So after read the code we find that it is most likely that the activeSessions_ which is ConcurrentHashMap type already have a same sessionid may explain the problem.

[wey-gu]

@mo-avatar are you interested in a proposal on fixing this in an elegant way, please?

[mo-avatar]

@mo-avatar are you interested in a proposal on fixing this in an elegant way, please?

I have thought it for a time, but have no idea how to fix it elegantly, If I figure out a way, I will make a pr.
By the way "Insert session to local cache failed" failed issue emerged again. Now may be we can say the frequency of the error is about once a week in our production after at least three time retry.

Additionally the SessionID is a int64_t not uint64 I think，My fault.

I’ll take a look if the nanoseconds way will help.

[wey-gu]

Cool, please take your time ❤.