Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implemented the StorageClient #59

Merged
merged 5 commits into from
Jan 22, 2019
Merged

Implemented the StorageClient #59

merged 5 commits into from
Jan 22, 2019

Conversation

sherman-the-tank
Copy link
Member

Issue #58

Farewell 2018, and Hello 2019!!

The implementation is partially done. We need to implement the re-try logic when the leader changes. The framework is there, just need to fill it up. Will do it in next PR

@nebula-community-bot
Copy link
Member

Build succeeded.

1 similar comment
@nebula-community-bot
Copy link
Member

Build succeeded.

Copy link
Contributor

@dangleptr dangleptr left a comment

Choose a reason for hiding this comment

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

Very Impressive and elegant implementation.
Please note #56 , i have added more response types there.

src/graph/StorageClient.h Outdated Show resolved Hide resolved
src/graph/StorageClient.h Outdated Show resolved Hide resolved
src/graph/StorageClient.h Show resolved Hide resolved
src/graph/StorageClient.inl Outdated Show resolved Hide resolved
src/graph/StorageClient.inl Outdated Show resolved Hide resolved
@sherman-the-tank
Copy link
Member Author

Thanks for reviewing the code and for many good points. Will revise the diff and update it soon

src/graph/StorageClient.inl Outdated Show resolved Hide resolved
src/graph/StorageClient.cpp Outdated Show resolved Hide resolved
src/graph/StorageClient.cpp Outdated Show resolved Hide resolved
src/graph/StorageClient.inl Outdated Show resolved Hide resolved
src/graph/StorageClient.inl Outdated Show resolved Hide resolved
@nebula-community-bot
Copy link
Member

Build succeeded.

Copy link
Contributor

@dangleptr dangleptr left a comment

Choose a reason for hiding this comment

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

Please rebase it on master. Some interfaces have been changed after #58

@nebula-community-bot
Copy link
Member

Build failed.

@dutor
Copy link
Contributor

dutor commented Jan 14, 2019

As @dangleptr said, the testing failed due to the changes in another patch. Please perform a rebase to fix this.

@sherman-the-tank
Copy link
Member Author

@dangleptr I changed codes in all response objects to failed_codes, which only contains the failed partition. Could you please double check I made all the logic changes correctly? Thanks a lot

Also, I could not find the place where to set the -1 as the partition id for the entire host failure. I'm afraid I might miss that part, because we want to return the error code for every single partition

@nebula-community-bot
Copy link
Member

Build failed.

@sherman-the-tank
Copy link
Member Author

Log_cas_test failed. I'll take care of it

Copy link
Contributor

@dangleptr dangleptr left a comment

Choose a reason for hiding this comment

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

The patch looks more clear now. LGTM.

For the whole failure, please refer QueryBaseProcessor.inl: 157
and QueryEdgePropsProcessor.cpp: 47.

src/graph/StorageClient.h Outdated Show resolved Hide resolved
@nebula-community-bot
Copy link
Member

Unit testing passed.

Copy link
Contributor

@dangleptr dangleptr left a comment

Choose a reason for hiding this comment

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

LGTM.

@dutor dutor merged commit 69050aa into vesoft-inc:master Jan 22, 2019
@dutor dutor deleted the storage-client branch January 22, 2019 02:56
yixinglu pushed a commit to yixinglu/nebula that referenced this pull request Feb 16, 2020
* Implemented the StorageClient

* Refactored StorageClient to remove duplicate and similar code

* Addressed @dangleptr's and @dutor's comments

* Refactored StorageClient to not re-use thrift response objects

* Addressed @dangleptr's comments and rebased
tong-hao pushed a commit to tong-hao/nebula that referenced this pull request Jun 1, 2021
* Implemented the StorageClient

* Refactored StorageClient to remove duplicate and similar code

* Addressed @dangleptr's and @dutor's comments

* Refactored StorageClient to not re-use thrift response objects

* Addressed @dangleptr's comments and rebased
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.

None yet

4 participants