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

Add proxy support to the dataset container #2512 #2513

Merged
merged 4 commits into from
Mar 14, 2023

Conversation

YANGBoSunning
Copy link
Contributor

@YANGBoSunning YANGBoSunning commented Mar 14, 2023

This PR adds an option of configuring the proxy service address for the dataset container should be added to the configuration file of helm

The dataset container of graphscope provides the function of obtaining sample dataset online, but there are many inconveniences in accessing the external network to obtain data in many Intranet environments.

Fixes #2512

@welcome
Copy link

welcome bot commented Mar 14, 2023

Thanks for submitting your first pull request! You are awesome! 🤗
Please make sure you have signed the CLA, as this is a mandatory check before a PR being merged.
Welcome to the GraphScope community! 🎉 You can meet the community on DingTalk or Slack.

@CLAassistant
Copy link

CLAassistant commented Mar 14, 2023

CLA assistant check
All committers have signed the CLA.

@@ -70,6 +70,9 @@ engines:
enabled: False
image:
name: dataset
# proxy:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you add a comment here for what is proxy for, and what's the target scenario, it makes people or developers like me could understand this more easily

nargs="?",
const="{}",
default="{}",
help="Proxy for worker dataset."
Copy link
Collaborator

Choose a reason for hiding this comment

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

And please make this help more meaningful. That would be very helpful.

@@ -144,6 +145,8 @@ def __init__(
self._preemptive = preemptive
self._vineyard_shared_mem = vineyard_shared_mem

self._dataset_proxy = dataset_proxy
Copy link
Collaborator

Choose a reason for hiding this comment

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

I prefer to let the dataset_proxy follow the logic of volumes, they are both json strings. i.e.

  • loads the str to json in here, not in the EngineCluster(dataset_proxy=json.loads(dataset_proxy), ...);
  • b64encode it in the chart then decode here.

@codecov-commenter
Copy link

codecov-commenter commented Mar 14, 2023

Codecov Report

Merging #2513 (e14f4da) into main (8db75b2) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #2513   +/-   ##
=======================================
  Coverage   72.62%   72.62%           
=======================================
  Files          88       88           
  Lines        9816     9816           
=======================================
  Hits         7129     7129           
  Misses       2687     2687           

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8db75b2...e14f4da. Read the comment docs.

2. add help info for argument --dataset_proxy
3. change logic of dataset_proxy, input with base64 and decode in EngineCluster's init function
@YANGBoSunning
Copy link
Contributor Author

  1. add comment for dataset proxy in values.yaml
  2. add help info for argument --dataset_proxy
  3. change logic of dataset_proxy, input with base64 and decode in EngineCluster's init function
    @siyuan0322

@siyuan0322
Copy link
Collaborator

LGTM, we could merge this after the CI check.

@siyuan0322 siyuan0322 merged commit 3b8e8eb into alibaba:main Mar 14, 2023
@siyuan0322
Copy link
Collaborator

Merged. Thanks for your contribution @YANGBoSunning !

@YANGBoSunning
Copy link
Contributor Author

Thanks for your review, good luck!

@YANGBoSunning
Copy link
Contributor Author

I'm sorry about that i missed one line code in last commit and i have add it in branch YANGBoSunning:pr-2512. Please fix it. @siyuan0322

@siyuan0322
Copy link
Collaborator

siyuan0322 commented Mar 15, 2023

I'm sorry about that i missed one line code in last commit and i have add it in branch YANGBoSunning:pr-2512. Please fix it. @siyuan0322

Got it 😂 , thank you


Pushed #2525

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.

Enable proxy settings for dataset in values of helm
4 participants