comment the graph::session module (Part 1) #3878
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Shylock-Hg
reviewed
Feb 10, 2022
src/graph/session/ClientSession.cpp
Outdated
| * This source code is licensed under Apache 2.0 License. | ||
| */ | ||
| // Copyright (c) 2020 vesoft inc. All rights reserved. | ||
|
|
a8c9c3f to
6082fe6
Compare
7065657 to
344bd32
Compare
Shylock-Hg
reviewed
Feb 11, 2022
src/graph/session/ClientSession.h
Outdated
| @@ -23,8 +23,15 @@ struct SpaceInfo { | |||
| meta::cpp2::SpaceDesc spaceDesc; | |||
| }; | |||
|
|
|||
| // A ClientSession is equivalent to a connection between a client and a server. | |||
There was a problem hiding this comment.
Session is not equivalent to connection. E.G. session could be saved in cluster even after connection broken.
Shylock-Hg
reviewed
Feb 11, 2022
| class ClientSession final { | ||
| public: | ||
| // Creates a new ClientSession. | ||
| // session: session obj used in RPC. | ||
| // metaClient: used to communicate with meta server. |
There was a problem hiding this comment.
Add The information of session will be persisted in meta server.
Shylock-Hg
reviewed
Feb 11, 2022
| @@ -143,7 +144,8 @@ void GraphSessionManager::removeSession(SessionID id) { | |||
| return; | |||
| } | |||
|
|
|||
| iter->second->markAllQueryKilled(); | |||
| iter->second->markAllQueryKilled(); // Before removing the session, all queries on the session | |||
There was a problem hiding this comment.
Put multiple-lines comment in front.
| @@ -217,7 +219,8 @@ void GraphSessionManager::updateSessionsToMeta() { | |||
| } | |||
| } | |||
|
|
|||
| auto handleKilledQueries = [this](auto&& resp) { | |||
| auto handleKilledQueries = [this](auto&& resp) { // There may be expired queries, and the | |||
yixinglu
reviewed
Feb 14, 2022
| */ | ||
| // Copyright (c) 2020 vesoft inc. All rights reserved. | ||
| // | ||
| // This source code is licensed under Apache 2.0 License. |
There was a problem hiding this comment.
why change the comment format? This changes maybe invalidate the license checker.
There was a problem hiding this comment.
This is the new comment requirement:
https://confluence.nebula-graph.io/pages/viewpage.action?pageId=35293986
The license check looks fine, it can pass.
src/graph/service/GraphService.cpp
Outdated
| @@ -80,7 +80,7 @@ folly::Future<AuthResponse> GraphService::future_authenticate(const std::string& | |||
| return future; | |||
| } | |||
|
|
|||
| if (!sessionManager_->isOutOfConnections()) { | |||
| if (!sessionManager_->isWithinConnections()) { | |||
There was a problem hiding this comment.
prefer the previous function name.
Shylock-Hg
previously approved these changes
Feb 14, 2022
Shylock-Hg
reviewed
Feb 14, 2022
src/graph/service/GraphService.cpp
Outdated
| @@ -80,7 +80,7 @@ folly::Future<AuthResponse> GraphService::future_authenticate(const std::string& | |||
| return future; | |||
| } | |||
|
|
|||
| if (!sessionManager_->isWithinConnections()) { | |||
| if (!sessionManager_->isOutOfConnections()) { | |||
There was a problem hiding this comment.
You should keep isOutOfConnections fit its name.
There was a problem hiding this comment.
yixinglu suggests isOutOfConnections prefer.
There was a problem hiding this comment.
The name of this function is obviously conflict with its implementation.
There was a problem hiding this comment.
Obviously, it should be:
if (isOutOfConnections) {
//report error
}
34fab66 to
0f1554f
Compare
Shylock-Hg
approved these changes
Feb 14, 2022
yixinglu
approved these changes
Feb 14, 2022
liwenhui-soul
pushed a commit
to liwenhui-soul/nebula
that referenced
this pull request
Feb 15, 2022
* comment GraphSessionManager * comment ClientSession * modify_and_comment_session * comment * add more comment * fix comment * fix comment * fix comment
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What type of PR is this?
What problem(s) does this PR solve?
Issue(s) number:
#3858
Description:
Just comment graph::session module.
How do you solve it?
Special notes for your reviewer, ex. impact of this fix, design document, etc:
Checklist:
Tests:
Affects:
Release notes:
Please confirm whether to be reflected in release notes and how to describe: