Conversation
71599e9 to
856cef0
Compare
ARCUSCTL_HOME 환경변수 설정 내부 메타데이터 저장 경로를 결정하는
export ARCUSCTL_HOME=/custom/path/.arcusctl |
namsic
left a comment
There was a problem hiding this comment.
리뷰 의견 반영된 것처럼 설정 파일 위치를 ARCUSCTL_HOME으로 지정할 수 있도록 하는 것이 좋아 보입니다.
실행 위치에 따라 base path를 달리하는 것은 현재 시점에서 크게 필요하지는 않습니다.
이미 ARCUSCTL_HOME 경로 하위에서 ensemble, servicecode 마다 하위 디렉토리를 두어 관리하는 구조이므로,
한 장비에서 여러 개의 ARCUSCTL_HOME을 두고 스위칭하는 패턴에 대한 요구는 크지 않을 것으로 생각합니다.
| panic(err) | ||
| } | ||
| defer conn.Close() | ||
| defer func() { _ = conn.Close() }() |
There was a problem hiding this comment.
아마 반환값을 명시적으로 처리하지 않아서 IDE에서 warning 발생한 것 같은데,
어떤 도구/설정에 의한 것인지 알아야 합니다.
기존 CI 테스트에서 이상 없었던 것으로 보아 golangci-lint 기본 설정에 의한 문제는 아닌 것으로 보이고,
이후 통일된 코드를 유지하려면 다른 개발 환경에서도 동일한 설정을 적용할 수 있도록 하거나
CI 테스트에 추가하여 관리해야 할 것입니다.
이러한 변경은 이번 PR에서 다룰 내용은 아니고, 별도로 처리해야 할 것 같습니다.
이와는 별개로, defer conn.Close()는 공식 문서나 예시에서도 일반적으로 쓰이는 표현이라
defer func() { _ = conn.Close() }()와 같이 하는 것이 조금 과한 느낌은 있습니다.
There was a problem hiding this comment.
@f1v3-dev
defer func() { _ = conn.Close() }()에 관한 위의 코멘트에 대해서도 간단히 답변하면 좋을 것 같습니다.
| if err := os.WriteFile(filepath.Join(dir, "meta.yml"), metaData, 0644); err != nil { | ||
| return err | ||
| } | ||
|
|
||
| return os.WriteFile(filepath.Join(dir, "topology.yml"), topologyData, 0644) | ||
| } | ||
|
|
||
| func LoadZKMeta(ensembleName string) (*ZKMeta, error) { | ||
| data, err := os.ReadFile(filepath.Join(arcusDir(), "clusters", "zk", ensembleName, "meta.yml")) |
There was a problem hiding this comment.
경로, 파일명 등 변동이 없는 문자열들을 상수로 분리하는 것은 어떤가요?
There was a problem hiding this comment.
실수를 줄이는 방향이라서 더 적합할 것 같습니다.
아래와 같은 예시를 기반으로 반영하고자 합니다.
as-is
func SaveZK(ensembleName string, version string, topologyData []byte) error {
dir := filepath.Join(arcusDir(), "clusters", "zk", ensembleName)to-be
const (
clusters = "clusters"
zk = "zk"
arcus = "arcus"
metaYML = "meta.yml"
topologyYML = "topology.yml"
)
...
func SaveZK(ensembleName string, version string, topologyData []byte) error {
dir := filepath.Join(arcusDir(), dirClusters, dirZK, ensembleName)혹시 변수명에 관한 네이밍 컨벤션같은게 있을까요?
There was a problem hiding this comment.
혹시 변수명에 관한 네이밍 컨벤션같은게 있을까요?
개인적으로 Effective Go를 참고하는 것을 제외하면 프로젝트 수준의 네이밍 컨벤션은 없습니다.
| func Copy(localPath string, host string, remotePath string) error { | ||
| username, err := currentUser() | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| dest := fmt.Sprintf("%s@%s:%s", username, host, remotePath) | ||
| cmd := exec.Command("scp", | ||
| "-i", os.Getenv("HOME")+"/.ssh/id_rsa", | ||
| "-o", "StrictHostKeyChecking=no", | ||
| localPath, | ||
| dest) | ||
| cmd.Stdout = os.Stdout | ||
| cmd.Stderr = os.Stderr | ||
| return cmd.Run() | ||
| } |
There was a problem hiding this comment.
zk deploy 명령어 실행 시 로컬에서 생성한 ZooKeeper 설정 파일인 zoo.cfg, zoo.cfg.dynamic 을 원격 서버로 전송(scp 명령어 사용)할 때 사용하게 됩니다.
|
논의 결과, go 버전 업그레이드 및 lint 관련 수정은 개발하는 환경에 따라 의존성 차이가 발생하는 것으로 보입니다.
따라서 현재 PR의 변경사항을 유지(go 버전 업그레이드 및 |
jhpark816
left a comment
There was a problem hiding this comment.
일부 리뷰
대부분 용어 등의 리팩토링에 관한 내용입니다.
| GlobalConfig ClusterConfig `yaml:"global_config"` | ||
| } | ||
|
|
||
| type ClusterServer struct { |
There was a problem hiding this comment.
@oliviarla @namsic
용어가 괜찮은가요?
MCServer, MemcServer, CacheServer 등이 대안입니다.
아래에 있는 코드를 보니, ZKServer, ZKConfig가 사용되고 있고
따라서 MCServer, MCConfig가 나을 거라고 생각됩니다.
| Role string `yaml:"role"` | ||
| } | ||
|
|
||
| type ClusterConfig struct { |
There was a problem hiding this comment.
여기도 개별 캐시 노드에 대한 설정이어서, 용어 검토가 필요합니다.
| type ClusterTopology struct { | ||
| ServiceCode string `yaml:"servicecode"` | ||
| Path string `yaml:"path"` | ||
| ZooKeeper string `yaml:"zookeeper"` |
There was a problem hiding this comment.
ServiceCode 다음에 ZooKeeper가 나오는 순서이면 좋겠습니다.
Path는 어떤 path인가요?
| const ( | ||
| clusters = "clusters" | ||
| zk = "zk" | ||
| arcus = "arcus" |
There was a problem hiding this comment.
"arcus" 라는 문자열 대신에 arcus 상수로 정의하여 사용하는 형태가 보편적인가요?
| } | ||
|
|
||
| func SaveZK(ensembleName string, version string, topologyData []byte) error { | ||
| dir := filepath.Join(arcusDir(), clusters, zk, ensembleName) |
There was a problem hiding this comment.
"clusters", "zk" 라는 문자열을 그대로 사용하는 것이 직관적이지 않는 지 ?
| } | ||
|
|
||
| return &meta, nil | ||
| } |
There was a problem hiding this comment.
ZKTopology loading 하는 함수를 여기에 두어도 되지 않는 지?
| } | ||
|
|
||
| return names, nil | ||
| } |
There was a problem hiding this comment.
아래 용어는 어떤지?
- ZK => ZKEnsemble
- Cluster => MCCluster
| panic(err) | ||
| } | ||
| defer conn.Close() | ||
| defer func() { _ = conn.Close() }() |
There was a problem hiding this comment.
@f1v3-dev
defer func() { _ = conn.Close() }()에 관한 위의 코멘트에 대해서도 간단히 답변하면 좋을 것 같습니다.
🔗 Related Issue
⌨️ What I did
arcusctl의 zk/cluster 관리 기능 구현을 위한 기반 구조를 추가합니다.
추가된 패키지
internal/topology: ZK 및 Clustertopology.yml파싱을 위한 구조체internal/store:~/.arcusctl/내부 메타데이터(meta.yml,topology.yml) 저장 및 읽기internal/ssh: SSH 원격 명령 실행 및 SCP 파일 전송추가된 커맨드
arcusctl zk: deploy / start / stop / delete / list / statusarcusctl cluster: deploy / start / stop / delete / list / status궁금한 점
현재 내부 메타데이터 저장 경로를 아래와 같이 고정해둔 상태입니다.
~/.arcusctl/clusters/zk/<ensemble-name>/~/.arcusctl/clusters/arcus/<servicecode>/위와 같이 홈 디렉토리(
~/.arcusctl)를 기준으로 고정해두는게 맞는지 의문이 있습니다. 어디서 실행해도 동일한 데이터를 보는 장점이 있지만, 실행 위치 기준으로 관리하게 된다면 환경 및 프로젝트 별로 분리가 가능할 것 같다고 보여집니다.