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.

Sign up for GitHub

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

Jump to bottom

feat: support http2 #246

Merged
merged 2 commits into from
Feb 1, 2023
Merged

feat: support http2 #246

merged 2 commits into from
Feb 1, 2023

Conversation

veezhang
Copy link
Contributor

@veezhang veezhang commented Jan 30, 2023 •
edited
Loading

What type of PR is this?

  • bug
  • feature
  • enhancement

What problem(s) does this PR solve?

Issue(s) number:

fix #245

Description:

How do you solve it?

Special notes for your reviewer, ex. impact of this fix, design document, etc:

@veezhang veezhang added the wip Working in progress. label Jan 30, 2023
@veezhang veezhang force-pushed the http2 branch 2 times, most recently from 21492fa to fd53aca Compare January 30, 2023 08:21
@veezhang veezhang removed the wip Working in progress. label Jan 30, 2023
@Aiee
Copy link
Contributor

Aiee commented Jan 31, 2023

The nebula service used in tests is not http2-based, we might need a separate docker image to test the http2 feature.

wenhaocs
wenhaocs approved these changes Jan 31, 2023
View reviewed changes
Copy link

@wenhaocs wenhaocs left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Good to me in terms of the HTTP support. Please take a look. Thanks! @Aiee

Aiee
Aiee reviewed Jan 31, 2023
View reviewed changes
go.mod Outdated
Comment on lines 11 to 12
// TODO: When confirmed, fork it to vesoft-inc.
replace github.com/facebook/fbthrift => github.com/veezhang/fbthrift v0.0.0-20230130031010-dc2d376142c6
Copy link
Contributor

Choose a reason for hiding this comment

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

Replace with official source

examples/basic_example/graph_client_basic_example.go
@@ -21,6 +22,8 @@ const (
port = 3699
username = "root"
password = "nebula"
useSSL = false
Copy link
Contributor

Choose a reason for hiding this comment

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

Please make sure these changes in examples will not be merged into the master branch, the examples should be concise.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding this doesn't make it too complicated, but he can tell users how to use ssl and http2. I think this is also beneficial? What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Of course, this will not be merged into master for the time being. If you think it is not necessary, I will delete it before merging into master.

@veezhang
Copy link
Contributor Author

image

@veezhang
Copy link
Contributor Author

veezhang commented Feb 1, 2023

vesoft-inc/fbthrift#1

@codecov-commenter
Copy link

Codecov Report

Base: 62.64% // Head: 62.00% // Decreases project coverage by -0.65% ⚠️

Coverage data is based on head (c6ed4a4) compared to base (f38f53c).
Patch coverage: 36.50% of modified lines in pull request are covered.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files 
@@                   Coverage Diff                   @@
##           release-3.4-beta-sc     #246      +/-   ##
=======================================================
- Coverage                62.64%   62.00%   -0.65%     
=======================================================
  Files                        9        9              
  Lines                     2230     2266      +36     
=======================================================
+ Hits                      1397     1405       +8     
- Misses                     693      720      +27     
- Partials                   140      141       +1
Impacted Files Coverage Δ
session.go 47.69% <ø> (ø)
value_wrapper.go 71.16% <ø> (ø)
configs.go 38.29% <20.00%> (-0.68%) ⬇️
connection.go 53.12% <34.78%> (-10.15%) ⬇️
connection_pool.go 80.70% <50.00%> (ø)
session_pool.go 62.03% <50.00%> (+0.12%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@Aiee Aiee merged commit c284022 into vesoft-inc:release-3.4-beta-sc Feb 1, 2023
@wey-gu wey-gu mentioned this pull request Feb 4, 2023
Closed
@wenhaocs wenhaocs linked an issue Feb 28, 2023 that may be closed by this pull request
Support HTTP in nebula go client #245
Closed
Sophie-Xie pushed a commit that referenced this pull request Jun 12, 2023
@veezhang @Sophie-Xie
feat: support http2 (#246)
8953ca8
veezhang added a commit to veezhang/nebula-go that referenced this pull request Jun 12, 2023
@veezhang
feat: support http2 (vesoft-inc#246)
7f1c2cd
Nicole00 pushed a commit that referenced this pull request Jun 13, 2023
@Sophie-Xie @veezhang
Cherry pick from 3.4-beta-sc: support http2 (#278)
8a5ad7e 
* feat: support http2 (#246)

* update fbthrift (#255)

* fix conflicts

* remove ssl examples

---------

Co-authored-by: Vee Zhang <veezhang@126.com>
Sophie-Xie added a commit that referenced this pull request Aug 8, 2023
@Sophie-Xie @veezhang
Cherry pick from 3.4-beta-sc: support http2 (#278)
278925f 
* feat: support http2 (#246)

* update fbthrift (#255)

* fix conflicts

* remove ssl examples

---------

Co-authored-by: Vee Zhang <veezhang@126.com>
veezhang added a commit that referenced this pull request Aug 23, 2023
@Sophie-Xie @veezhang
Cherry pick from 3.4-beta-sc: support http2 (#278)
21b65af 
* feat: support http2 (#246)

* update fbthrift (#255)

* fix conflicts

* remove ssl examples

---------

Co-authored-by: Vee Zhang <veezhang@126.com>
Nicole00 pushed a commit that referenced this pull request Aug 23, 2023
@Sophie-Xie @veezhang
support http2 (#278) (#280)
d79d26b 
* Cherry pick from 3.4-beta-sc:  support http2 (#278)

* feat: support http2 (#246)

* update fbthrift (#255)

* fix conflicts

* remove ssl examples

---------

Co-authored-by: Vee Zhang <veezhang@126.com>

* use vesoft-inc/fbthrift

---------

Co-authored-by: Vee Zhang <veezhang@126.com>
veezhang added a commit to veezhang/nebula-go that referenced this pull request Sep 5, 2023
@veezhang
feat: support http2 (vesoft-inc#246)
9fe7a1b
@songqing songqing mentioned this pull request Feb 23, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Aiee Aiee Aiee left review comments

@wenhaocs wenhaocs wenhaocs approved these changes

@yixinglu yixinglu Awaiting requested review from yixinglu

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support HTTP in nebula go client
4 participants
@veezhang @Aiee @codecov-commenter @wenhaocs