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

OSPP2023-23aaf0426 #65

Merged
merged 7 commits into from
Oct 14, 2023
Merged

OSPP2023-23aaf0426 #65

merged 7 commits into from
Oct 14, 2023

Conversation

niuniudeadams
Copy link

Describe what this PR does / why we need it

Does this pull request fix one issue?

Describe how you did it

Describe how to verify it

Special notes for reviews

@CLAassistant
Copy link

CLAassistant commented Aug 10, 2023

CLA assistant check
All committers have signed the CLA.

@niuniudeadams niuniudeadams changed the base branch from main to summer August 10, 2023 02:49
@SpecialYang SpecialYang self-requested a review August 10, 2023 02:50
Copy link
Collaborator

@SpecialYang SpecialYang left a comment

Choose a reason for hiding this comment

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

整体逻辑都没问题。先按意见改一下。
另外把pr的标题从first commint 改成 OSPP2023-{项目id}

control_plane.go Outdated
@@ -15,21 +15,28 @@
package opensergo

import (
discovery "github.com/envoyproxy/go-control-plane/envoy/service/discovery/v3"
Copy link
Collaborator

Choose a reason for hiding this comment

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

所有的import 都需要保持以下格式

基础库

第三方库

该项目

control_plane.go Outdated
if err != nil {
return nil, err
}

cp.server = transport.NewServer(uint32(10246), []model.SubscribeRequestHandler{cp.handleSubscribeRequest})
cp.xdsServer = transport.NewDiscoveryServer(uint32(8002), []model.SubscribeXDsRequestHandler{cp.handleXDSSubscribeRequest})
Copy link
Collaborator

Choose a reason for hiding this comment

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

这个端口是你自定义的?

Copy link
Author

Choose a reason for hiding this comment

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

是的,因为我看原项目也是固定端口是10246,所以我就写了8002

Copy link
Collaborator

Choose a reason for hiding this comment

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

你看看10248 有没有用?没有,就用这个吧

Copy link
Author

Choose a reason for hiding this comment

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

好的,我看一下

func (conn *XDSConnection) Watched(typeUrl string) *WatchedResource {
conn.RLock()
defer conn.RUnlock()
if conn.WatchedResources != nil && conn.WatchedResources[typeUrl] != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

WatchedResources 不会有nil的情况

control_plane.go Outdated
crdWatcher, err := c.operator.RegisterWatcher(model.SubscribeTarget{
Namespace: request[4],
AppName: request[3],
Kind: request[0] + delimiter + request[1] + delimiter + request[2],
Copy link
Collaborator

Choose a reason for hiding this comment

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

这里 数字的可维护性太差了,建议弄一个函数包一下。
namespace, appname, kind := tool(xxxx)

control_plane.go Outdated
}

// Add the connection to the connection map.
c.xdsServer.AddConnectionToMap(request[4], request[3], request[0]+"/"+request[1]+"/"+request[2], con)
Copy link
Collaborator

Choose a reason for hiding this comment

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

这里同理,建议,在最上面提前计算好

@@ -1,4 +1,3 @@

Copy link
Collaborator

Choose a reason for hiding this comment

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

没必要的改动

Copy link
Collaborator

Choose a reason for hiding this comment

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

这里的改动是什么?

Copy link
Author

Choose a reason for hiding this comment

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

我可能删了第一个空行,导师

@@ -15,17 +15,18 @@
package main

import (
Copy link
Collaborator

Choose a reason for hiding this comment

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

以下所有的改动,都没必要

for {
// Go select{} statements are not ordered; the same channel can be chosen many times.
// For requests, these are higher priority (client may be blocked on startup until these are done)
// and often very cheap to handle (simple ACK), so we check it first.
Copy link
Collaborator

Choose a reason for hiding this comment

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

这个注释没必要了

@niuniudeadams niuniudeadams changed the title first commit OSPP2023-23aaf0426 Oct 8, 2023
Copy link
Collaborator

@SpecialYang SpecialYang left a comment

Choose a reason for hiding this comment

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

LGTM.

@SpecialYang SpecialYang merged commit a005992 into opensergo:summer Oct 14, 2023
3 of 4 checks passed
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

3 participants