Skip to content

Commit

Permalink
Address review comments by @ishan16696
Browse files Browse the repository at this point in the history
  • Loading branch information
@anveshreddy18
anveshreddy18 committed Nov 8, 2024
1 parent f303bb3 commit 2f788e2
Show file tree
Hide file tree
Showing 5 changed files with 27 additions and 27 deletions.
6 changes: 3 additions & 3 deletions pkg/member/member_control.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ func NewMemberControl(etcdConnConfig *brtypes.EtcdConnectionConfig) Control {
// AddMemberAsLearner add a member as a learner to the etcd cluster
func (m *memberControl) AddMemberAsLearner(ctx context.Context) error {
//Add member as learner to cluster
memberPeerURLs, err := miscellaneous.GetInitialAdvertisePeerURLs(m.configFile)
memberPeerURLs, err := miscellaneous.GetMemberPeerURLs(m.configFile)
if err != nil {
m.logger.Fatalf("Error fetching etcd member URL : %v", err)
}
Expand All @@ -133,7 +133,7 @@ func (m *memberControl) AddMemberAsLearner(ctx context.Context) error {
response, err := cli.MemberAddAsLearner(memAddCtx, memberPeerURLs)
if err != nil {
if errors.Is(err, rpctypes.Error(rpctypes.ErrGRPCPeerURLExist)) || errors.Is(err, rpctypes.Error(rpctypes.ErrGRPCMemberExist)) {
m.logger.Infof("Member %s already part of etcd cluster", m.podName)
m.logger.Infof("Member %s with peer urls %v already part of etcd cluster", m.podName, memberPeerURLs)
return nil
} else if errors.Is(err, rpctypes.Error(rpctypes.ErrGRPCTooManyLearners)) {
m.logger.Infof("Unable to add member %s as a learner because the cluster already has a learner", m.podName)
Expand Down Expand Up @@ -205,7 +205,7 @@ func (m *memberControl) IsMemberInCluster(ctx context.Context) (bool, error) {
func (m *memberControl) doUpdateMemberPeerAddress(ctx context.Context, cli etcdClient.ClusterCloser, id uint64) error {
// Already existing clusters or cluster after restoration have `http://localhost:2380` as the peer address. This needs to explicitly updated to the correct peer address.
m.logger.Infof("Updating member peer URL for %s", m.podName)
memberPeerURLs, err := miscellaneous.GetInitialAdvertisePeerURLs(m.configFile)
memberPeerURLs, err := miscellaneous.GetMemberPeerURLs(m.configFile)
if err != nil {
return fmt.Errorf("could not fetch member URL : %v", err)
}
Expand Down
14 changes: 7 additions & 7 deletions pkg/miscellaneous/miscellaneous.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -63,8 +63,8 @@ const (
)

type advertiseURLsConfig struct {
AdvertiseClientURLs map[string][]string `yaml:"advertise-client-urls"`
InitialAdvertisePeerURLs map[string][]string `yaml:"initial-advertise-peer-urls"`
AdvertiseClientURLs map[string][]string `json:"advertise-client-urls"`
InitialAdvertisePeerURLs map[string][]string `json:"initial-advertise-peer-urls"`
}

// GetLatestFullSnapshotAndDeltaSnapList returns the latest snapshot
Expand Down Expand Up @@ -554,8 +554,8 @@ func parseAdvertiseURLsConfig(configFile string) (*advertiseURLsConfig, error) {
return &advURLsConfig, nil
}

// GetInitialAdvertisePeerURLs retrieves the initial advertise peer URLs for the etcd member using the POD_NAME environment variable.
func GetInitialAdvertisePeerURLs(configFile string) ([]string, error) {
// GetMemberPeerURLs retrieves the initial advertise peer URLs for the etcd member using the POD_NAME environment variable.
func GetMemberPeerURLs(configFile string) ([]string, error) {
memberName, err := GetEnvVarOrError("POD_NAME")
if err != nil {
return nil, err
Expand All @@ -579,8 +579,8 @@ func GetInitialAdvertisePeerURLs(configFile string) ([]string, error) {
return peerURLs, nil
}

// GetAdvertiseClientURLs retrieves the advertise client URLs for the etcd member using the POD_NAME environment variable.
func GetAdvertiseClientURLs(configFile string) ([]string, error) {
// GetMemberClientURLs retrieves the advertise client URLs for the etcd member using the POD_NAME environment variable.
func GetMemberClientURLs(configFile string) ([]string, error) {
memberName, err := GetEnvVarOrError("POD_NAME")
if err != nil {
return nil, err
Expand All @@ -606,7 +606,7 @@ func GetAdvertiseClientURLs(configFile string) ([]string, error) {

// IsPeerURLTLSEnabled checks whether all peer URLs are TLS-enabled (i.e., use the "https" scheme).
func IsPeerURLTLSEnabled() (bool, error) {
memberPeerURLs, err := GetInitialAdvertisePeerURLs(GetConfigFilePath())
memberPeerURLs, err := GetMemberPeerURLs(GetConfigFilePath())
if err != nil {
return false, fmt.Errorf("failed to get initial advertise peer URLs: %w", err)
}
Expand Down
28 changes: 14 additions & 14 deletions pkg/miscellaneous/miscellaneous_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -598,8 +598,8 @@ var _ = Describe("Miscellaneous Tests", func() {
_, err := function(configFile)
Expect(err).To(HaveOccurred())
},
Entry("GetInitialAdvertisePeerURLs", "initial-advertise-peer-urls", GetInitialAdvertisePeerURLs),
Entry("GetAdvertiseClientURLs", "advertise-client-urls", GetAdvertiseClientURLs),
Entry("GetMemberPeerURLs", "initial-advertise-peer-urls", GetMemberPeerURLs),
Entry("GetMemberClientURLs", "advertise-client-urls", GetMemberClientURLs),
)
})

Expand All @@ -618,8 +618,8 @@ var _ = Describe("Miscellaneous Tests", func() {
_, err := function(configFile)
Expect(err).To(HaveOccurred())
},
Entry("GetInitialAdvertisePeerURLs", "initial-advertise-peer-urls", GetInitialAdvertisePeerURLs),
Entry("GetAdvertiseClientURLs", "advertise-client-urls", GetAdvertiseClientURLs),
Entry("GetMemberPeerURLs", "initial-advertise-peer-urls", GetMemberPeerURLs),
Entry("GetMemberClientURLs", "advertise-client-urls", GetMemberClientURLs),
)
})

Expand All @@ -641,8 +641,8 @@ var _ = Describe("Miscellaneous Tests", func() {
_, err := function(configFile)
Expect(err).To(HaveOccurred())
},
Entry("GetInitialAdvertisePeerURLs", "initial-advertise-peer-urls", GetInitialAdvertisePeerURLs),
Entry("GetAdvertiseClientURLs", "advertise-client-urls", GetAdvertiseClientURLs),
Entry("GetMemberPeerURLs", "initial-advertise-peer-urls", GetMemberPeerURLs),
Entry("GetMemberClientURLs", "advertise-client-urls", GetMemberClientURLs),
)
})

Expand All @@ -669,8 +669,8 @@ var _ = Describe("Miscellaneous Tests", func() {
_, err := function(configFile)
Expect(err).To(HaveOccurred())
},
Entry("GetInitialAdvertisePeerURLs", "initial-advertise-peer-urls", GetInitialAdvertisePeerURLs),
Entry("GetAdvertiseClientURLs", "advertise-client-urls", GetAdvertiseClientURLs),
Entry("GetMemberPeerURLs", "initial-advertise-peer-urls", GetMemberPeerURLs),
Entry("GetMemberClientURLs", "advertise-client-urls", GetMemberClientURLs),
)
})

Expand All @@ -689,8 +689,8 @@ var _ = Describe("Miscellaneous Tests", func() {
_, err := function(configFile)
Expect(err).To(HaveOccurred())
},
Entry("GetInitialAdvertisePeerURLs", "initial-advertise-peer-urls", GetInitialAdvertisePeerURLs),
Entry("GetAdvertiseClientURLs", "advertise-client-urls", GetAdvertiseClientURLs),
Entry("GetMemberPeerURLs", "initial-advertise-peer-urls", GetMemberPeerURLs),
Entry("GetMemberClientURLs", "advertise-client-urls", GetMemberClientURLs),
)
})

Expand All @@ -710,8 +710,8 @@ var _ = Describe("Miscellaneous Tests", func() {
_, err := function(configFile)
Expect(err).To(HaveOccurred())
},
Entry("GetInitialAdvertisePeerURLs", "initial-advertise-peer-urls", GetInitialAdvertisePeerURLs),
Entry("GetAdvertiseClientURLs", "advertise-client-urls", GetAdvertiseClientURLs),
Entry("GetMemberPeerURLs", "initial-advertise-peer-urls", GetMemberPeerURLs),
Entry("GetMemberClientURLs", "advertise-client-urls", GetMemberClientURLs),
)
})

Expand All @@ -731,8 +731,8 @@ var _ = Describe("Miscellaneous Tests", func() {
Expect(err).To(Not(HaveOccurred()))
Expect(urls).To(Equal(podUrlsMap[podName]))
},
Entry("GetInitialAdvertisePeerURLs", "initial-advertise-peer-urls", GetInitialAdvertisePeerURLs),
Entry("GetAdvertiseClientURLs", "advertise-client-urls", GetAdvertiseClientURLs),
Entry("GetMemberPeerURLs", "initial-advertise-peer-urls", GetMemberPeerURLs),
Entry("GetMemberClientURLs", "advertise-client-urls", GetMemberClientURLs),
)
})
})
Expand Down
2 changes: 1 addition & 1 deletion pkg/server/backuprestoreserver.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -571,7 +571,7 @@ func handleSsrStopRequest(ctx context.Context, handler *HTTPHandler, _ *snapshot
}

func hasPeerURLChanged(ctx context.Context, m member.Control, cli client.ClusterCloser) (bool, error) {
peerURLsFromEtcdConfig, err := miscellaneous.GetInitialAdvertisePeerURLs(miscellaneous.GetConfigFilePath())
peerURLsFromEtcdConfig, err := miscellaneous.GetMemberPeerURLs(miscellaneous.GetConfigFilePath())
if err != nil {
return false, fmt.Errorf("failed to get initial advertise peer URLs: %w", err)
}
Expand Down
4 changes: 2 additions & 2 deletions pkg/server/httpAPI.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -429,7 +429,7 @@ func (h *HTTPHandler) serveConfig(rw http.ResponseWriter, req *http.Request) {
config["name"] = podName

// fetch initial-advertise-peer-urls from etcd config file
initAdPeerURLs, err := miscellaneous.GetInitialAdvertisePeerURLs(inputFileName)
initAdPeerURLs, err := miscellaneous.GetMemberPeerURLs(inputFileName)
if err != nil {
h.Logger.Warnf("Unable to get initial-advertise-peer-urls from etcd config file: %v", err)
rw.WriteHeader(http.StatusInternalServerError)
Expand All @@ -438,7 +438,7 @@ func (h *HTTPHandler) serveConfig(rw http.ResponseWriter, req *http.Request) {
config["initial-advertise-peer-urls"] = strings.Join(initAdPeerURLs, ",")

// fetch advertise-client-urls from etcd config file
advClientURLs, err := miscellaneous.GetAdvertiseClientURLs(inputFileName)
advClientURLs, err := miscellaneous.GetMemberClientURLs(inputFileName)
if err != nil {
h.Logger.Warnf("Unable to get advertise-client-urls from etcd config file: %v", err)
rw.WriteHeader(http.StatusInternalServerError)
Expand Down

0 comments on commit 2f788e2

Please sign in to comment.