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

update GraphStorageClient.py #138

Merged
merged 8 commits into from
Sep 23, 2021
Merged

update GraphStorageClient.py #138

merged 8 commits into from
Sep 23, 2021

Conversation

kevin-meng
Copy link
Contributor

  1. 更新 GraphStorageClient.py 中的类 GraphStorageClient。
  2. 为此 修改 readme.md 样例。

原因: 使用 nebula-docker-compose 拉起环境后,宿主机上安装 nebula-python 在运行第二个样例时报错:
InValidHostname: storaged0,宿主机无法识别 storaged0

解决方案

通过手动配置 storage_addrs,并将其作为参数传入其中,来解决。
使用if条件判断,提供更多自由度

@CLAassistant
Copy link

CLAassistant commented Sep 13, 2021

CLA assistant check
All committers have signed the CLA.

@wey-gu
Copy link
Contributor

wey-gu commented Sep 14, 2021

Dear @kevin-meng!
Thank you for the PR!
这里原来的理念是 storage host list 是动态的,而且是被 metaD 来发现、维护的,所以它应该从 meta 去获取。

一般来说,如果有 storageClient 的使用场景,我们需要讲 storageD 的地址暴露给 client,一个方法是修改 storageD 的配置(docker-compose 的情况下,就是修改 compose yaml 文件),另一个方法(docker compose)是使得 client 运行在 docker 网络里,这样默认配置的 storage0 就是可以访问、可解析的了哈。

您这么修改很聪明,直接找到了问题所在👍🏻,不过它代价的是 meta 对 stroageD 集群的自动发现,使得 storageD host list 的信息需要程序去自动维护(一般来说 meta 的 host list是固定的、storageD 是浮动的)。
所以,我认为为 storageClient 配置可人为指定 host list 的选择是很好的👍🏻,但是在例子里应该成为第二选择,而不是替换现有的推荐的方式(不过原来这里确实可以增加一下介绍,免除比如默认 compose 就连不上的 confuse),您觉得呢?

cc @yixinglu

@@ -111,15 +111,19 @@ connection_pool.close()
You should make sure the scan client can connect to the address of storage which see from `SHOW HOSTS`

```python
from nebula2.mclient import MetaCache
Copy link
Contributor

Choose a reason for hiding this comment

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

  • As mentioned in the conversation, it's better not to recommend manually filling storage addresses over the auto-discovering via metaD, instead, if we introduce this ability(it means application/user has to discover/maintain this list themselves), we should provide it in examples as an Option-B(second option).

  • Please add extra spaces after the comma:
    a, b is better than a,b

@@ -34,11 +34,13 @@ class GraphStorageClient(object):
DEFAULT_END_TIME = sys.maxsize
DEFAULT_LIMIT = 1000

def __init__(self, meta_cache, time_out=60000):
def __init__(self, meta_cache,storage_addrs =None, time_out=60000):
Copy link
Contributor

@wey-gu wey-gu Sep 14, 2021

Choose a reason for hiding this comment

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

def __init__(self, meta_cache, storage_addrs=None, time_out=60000):

self._time_out = time_out
self._connections = []
self._create_connection()

Copy link
Contributor

Choose a reason for hiding this comment

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

there should be only one newline between two functions inside a class

@wey-gu
Copy link
Contributor

wey-gu commented Sep 14, 2021

Also kindly sign the CLA, please :)

@kevin-meng
Copy link
Contributor Author

Dear @kevin-meng!
Thank you for the PR!
这里原来的理念是 storage host list 是动态的,而且是被 metaD 来发现、维护的,所以它应该从 meta 去获取。

一般来说,如果有 storageClient 的使用场景,我们需要讲 storageD 的地址暴露给 client,一个方法是修改 storageD 的配置(docker-compose 的情况下,就是修改 compose yaml 文件),另一个方法(docker compose)是使得 client 运行在 docker 网络里,这样默认配置的 storage0 就是可以访问、可解析的了哈。

您这么修改很聪明,直接找到了问题所在👍🏻,不过它代价的是 meta 对 stroageD 集群的自动发现,使得 storageD host list 的信息需要程序去自动维护(一般来说 meta 的 host list是固定的、storageD 是浮动的)。
所以,我认为为 storageClient 配置可人为指定 host list 的选择是很好的👍🏻,但是在例子里应该成为第二选择,而不是替换现有的推荐的方式(不过原来这里确实可以增加一下介绍,免除比如默认 compose 就连不上的 confuse),您觉得呢?

cc @yixinglu

认同。
第一次给开源项目提交 PR,就得到真么认真的对待,备受鼓励,哈哈。身为非专业开发人员,代码有些不够规范的地方,还请见谅~

@wey-gu
Copy link
Contributor

wey-gu commented Sep 15, 2021

认同。
第一次给开源项目提交 PR,就得到真么认真的对待,备受鼓励,哈哈。身为非专业开发人员,代码有些不够规范的地方,还请见谅~

您的 PR 很认真,我们不能怠慢哈,大家都是通过参加社区规范自己的代码的哈 👍🏻。我的建议是这样的哈,您可以 revise 这个commit 做如下修改

  • Readme:改成首选原来的例子,然后把您的例子放在第二个
  • GraphStorageClient.py:修改一下格式

您可以先试试,如果您不方便、不熟悉 github 做修改,没关系,我可以 co-author 来帮您改哈。

谢谢!
思为,敬上

@kevin-meng
Copy link
Contributor Author

认同。
第一次给开源项目提交 PR,就得到真么认真的对待,备受鼓励,哈哈。身为非专业开发人员,代码有些不够规范的地方,还请见谅~

您的 PR 很认真,我们不能怠慢哈,大家都是通过参加社区规范自己的代码的哈 👍🏻。我的建议是这样的哈,您可以 revise 这个commit 做如下修改

  • Readme:改成首选原来的例子,然后把您的例子放在第二个
  • GraphStorageClient.py:修改一下格式

您可以先试试,如果您不方便、不熟悉 github 做修改,没关系,我可以 co-author 来帮您改哈。

谢谢!
思为,敬上

已经尝试修改,并提交。如果 readme 的修改有不当的地方,就烦请 co-author 帮忙修改下哈,感谢。

README.md Outdated
@@ -111,16 +111,24 @@ connection_pool.close()
You should make sure the scan client can connect to the address of storage which see from `SHOW HOSTS`

```python
from nebula2.mclient import MetaCache
from nebula2.mclient import MetaCache,HostAddr
Copy link
Contributor

Choose a reason for hiding this comment

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

Space after the comma :)

from nebula2.mclient import MetaCache, HostAddr

README.md Outdated
graph_storage_client = GraphStorageClient(meta_cache)

# option 2 manually specify the storage address
storage_addrs = [HostAddr(host='172.28.1.4',port=9779),
Copy link
Contributor

Choose a reason for hiding this comment

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

storage_addrs = [HostAddr(host='172.28.1.4', port=9779),
                 HostAddr(host='172.28.1.5', port=9779),
                 HostAddr(host='172.28.1.6', port=9779)]

README.md Outdated
storage_addrs = [HostAddr(host='172.28.1.4',port=9779),
HostAddr(host='172.28.1.5',port=9779),
HostAddr(host='172.28.1.6',port=9779)]
graph_storage_client = GraphStorageClient(meta_cache,storage_addrs)
Copy link
Contributor

Choose a reason for hiding this comment

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

graph_storage_client = GraphStorageClient(meta_cache, storage_addrs)

self._meta_cache = meta_cache
self.storage_addrs = storage_addrs
Copy link
Contributor

Choose a reason for hiding this comment

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

please kindly use self._storage_addrs = storage_addrs, as it's intended for internal use only.

@wey-gu
Copy link
Contributor

wey-gu commented Sep 16, 2021

抱歉,我好像没有权限push,否则我就直接改了,有一点小的 linting 改动和 naming 改动给您 comment 了。
ref: wey-gu@01dea38

@kevin-meng
Copy link
Contributor Author

kevin-meng commented Sep 16, 2021

我重新提交了,真是太不好意思了...

Copy link
Contributor

@wey-gu wey-gu left a comment

Choose a reason for hiding this comment

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

感谢 Kevin :)
LGTM

@wey-gu wey-gu requested a review from Aiee September 17, 2021 06:41
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.

4 participants