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

Added retry 'registry' chain element #1202

Merged
merged 3 commits into from
Dec 24, 2021

Conversation

sol-0
Copy link
Contributor

@sol-0 sol-0 commented Dec 20, 2021

Signed-off-by: Oleg Solodkov oleg.solodkov@xored.com

Issue link

Fixes #1201

How Has This Been Tested?

  • Added unit testing to cover
  • Tested manually
  • Tested by integration testing
  • Have not tested

Types of changes

  • Bug fix
  • New functionallity
  • Documentation
  • Refactoring
  • CI

Signed-off-by: Oleg Solodkov <oleg.solodkov@xored.com>
@sol-0 sol-0 marked this pull request as ready for review December 20, 2021 16:28

// Option is NS/NSE retry client config option
type Option interface {
apply(configurable)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
apply(configurable)
apply(*options)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Comment on lines 21 to 24
type configurable interface {
setInterval(time.Duration)
setTryTimeout(time.Duration)
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
type configurable interface {
setInterval(time.Duration)
setTryTimeout(time.Duration)
}
type options interface {
internal time.Duration
timeout time.Duration
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Comment on lines 31 to 35
type applier func(configurable)

func (f applier) apply(c configurable) {
f(c)
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
type applier func(configurable)
func (f applier) apply(c configurable) {
f(c)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Comment on lines 35 to 36
registers, unregisters, finds map[string]int32
mu sync.Mutex
Copy link
Member

Choose a reason for hiding this comment

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

These fields are not used. Please don't add dead code.

Suggested change
registers, unregisters, finds map[string]int32
mu sync.Mutex

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

)

// NSEClient is a client type for counting Register / Unregister / Find
type NSEClient struct {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
type NSEClient struct {
type countNSEClient struct {

Copy link
Member

Choose a reason for hiding this comment

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

This can be private.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done


// NSEClient is a client type for counting Register / Unregister / Find
type NSEClient struct {
totalRegisterCalls, totalUnregisterCalls, totalFindCalls int32
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
totalRegisterCalls, totalUnregisterCalls, totalFindCalls int32
totalRegisterCalls, totalUnregisterCalls, totalFindCalls *int32

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Comment on lines 81 to 94
// Registers returns Register call count
func (c *NSEClient) Registers() int {
return int(atomic.LoadInt32(&c.totalRegisterCalls))
}

// Unregisters returns Unregister count
func (c *NSEClient) Unregisters() int {
return int(atomic.LoadInt32(&c.totalUnregisterCalls))
}

// Finds returns Find count
func (c *NSEClient) Finds() int {
return int(atomic.LoadInt32(&c.totalFindCalls))
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Registers returns Register call count
func (c *NSEClient) Registers() int {
return int(atomic.LoadInt32(&c.totalRegisterCalls))
}
// Unregisters returns Unregister count
func (c *NSEClient) Unregisters() int {
return int(atomic.LoadInt32(&c.totalUnregisterCalls))
}
// Finds returns Find count
func (c *NSEClient) Finds() int {
return int(atomic.LoadInt32(&c.totalFindCalls))
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How do we assert Register/Unregister/Find call count in tests?

Copy link
Member

Choose a reason for hiding this comment

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

@sol-0 Does #1202 (comment) resolve your question?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@denis-tingaikin yes, thanks! Done

registers, unregisters, finds map[string]int32
mu sync.Mutex
}

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
func NewNetworkServiceEndpointClinet(opts...options) registry.NetworkServiceEndpointClient {
var result = &countNSEClient{
totalRegisterCalls: new(int32)
//TODO totalUnregisterCalls, totalFindCalls
}
for _, opt := range options {
opt(result)
}
return result
}

where options

WithRegisters(r *int32) func(*options)
....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@denis-tingaikin
Copy link
Member

@sol-0 Please ping me when you've resolved all comments.

Signed-off-by: Oleg Solodkov <oleg.solodkov@xored.com>
@sol-0
Copy link
Contributor Author

sol-0 commented Dec 22, 2021

@denis-tingaikin, comments have been addressed - please, take a look.

Comment on lines 19 to 45
type options struct {
totalRegisterCalls, totalUnregisterCalls, totalFindCalls *int32
}

// Option is an option pattern for NewNetworkServiceRegistryClient, NewNetworkServiceEndpointRegistryClient
type Option func(*options)

// WithTotalRegisterCalls sets pointer to number of Register calls
func WithTotalRegisterCalls(registerCount *int32) func(*options) {
return func(o *options) {
o.totalRegisterCalls = registerCount
}
}

// WithTotalUnregisterCalls sets pointer to number of Unregister calls
func WithTotalUnregisterCalls(unregisterCount *int32) func(*options) {
return func(o *options) {
o.totalUnregisterCalls = unregisterCount
}
}

// WithTotalFindCalls sets pointer to number of Find calls
func WithTotalFindCalls(findCalls *int32) func(*options) {
return func(o *options) {
o.totalFindCalls = findCalls
}
}
Copy link
Member

@denis-tingaikin denis-tingaikin Dec 24, 2021

Choose a reason for hiding this comment

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

Suggested change
type options struct {
totalRegisterCalls, totalUnregisterCalls, totalFindCalls *int32
}
// Option is an option pattern for NewNetworkServiceRegistryClient, NewNetworkServiceEndpointRegistryClient
type Option func(*options)
// WithTotalRegisterCalls sets pointer to number of Register calls
func WithTotalRegisterCalls(registerCount *int32) func(*options) {
return func(o *options) {
o.totalRegisterCalls = registerCount
}
}
// WithTotalUnregisterCalls sets pointer to number of Unregister calls
func WithTotalUnregisterCalls(unregisterCount *int32) func(*options) {
return func(o *options) {
o.totalUnregisterCalls = unregisterCount
}
}
// WithTotalFindCalls sets pointer to number of Find calls
func WithTotalFindCalls(findCalls *int32) func(*options) {
return func(o *options) {
o.totalFindCalls = findCalls
}
}
type Counter struct {
totalRegisterCalls int32
totalUnregisterCalls int32
totalFindCalls int32
}
// Registers returns Register call count
func (c *Counter) Registers() int {
return int(atomic.LoadInt32(&c.totalRegisterCalls))
}
// Unregisters returns Unregister count
func (c *Counter) Unregisters() int {
return int(atomic.LoadInt32(&c.totalUnregisterCalls))
}
// Finds returns Find count
func (c *Counter) Finds() int {
return int(atomic.LoadInt32(&c.totalFindCalls))
}
// TODO: add this to the constructor of count servers

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Signed-off-by: Oleg Solodkov <oleg.solodkov@xored.com>
@denis-tingaikin denis-tingaikin merged commit 29ac91f into networkservicemesh:main Dec 24, 2021
nsmbot pushed a commit to networkservicemesh/sdk-k8s that referenced this pull request Dec 24, 2021
…k@main

PR link: networkservicemesh/sdk#1202

Commit: 29ac91f
Author: Sol-0
Date: 2021-12-24 20:21:38 +0700
Message:
  - Added retry 'registry' chain element (#1202)
* Added retry registry client chain element

Signed-off-by: Oleg Solodkov <oleg.solodkov@xored.com>

* Addressed review comments

Signed-off-by: Oleg Solodkov <oleg.solodkov@xored.com>

* Addressed review comments

Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
nsmbot pushed a commit to networkservicemesh/sdk-kernel that referenced this pull request Dec 24, 2021
…k@main

PR link: networkservicemesh/sdk#1202

Commit: 29ac91f
Author: Sol-0
Date: 2021-12-24 20:21:38 +0700
Message:
  - Added retry 'registry' chain element (#1202)
* Added retry registry client chain element

Signed-off-by: Oleg Solodkov <oleg.solodkov@xored.com>

* Addressed review comments

Signed-off-by: Oleg Solodkov <oleg.solodkov@xored.com>

* Addressed review comments

Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
nsmbot pushed a commit to networkservicemesh/cmd-admission-webhook-k8s that referenced this pull request Dec 24, 2021
…k@main

PR link: networkservicemesh/sdk#1202

Commit: 29ac91f
Author: Sol-0
Date: 2021-12-24 20:21:38 +0700
Message:
  - Added retry 'registry' chain element (#1202)
* Added retry registry client chain element

Signed-off-by: Oleg Solodkov <oleg.solodkov@xored.com>

* Addressed review comments

Signed-off-by: Oleg Solodkov <oleg.solodkov@xored.com>

* Addressed review comments

Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
nsmbot pushed a commit to networkservicemesh/cmd-registry-proxy-dns that referenced this pull request Dec 24, 2021
…k@main

PR link: networkservicemesh/sdk#1202

Commit: 29ac91f
Author: Sol-0
Date: 2021-12-24 20:21:38 +0700
Message:
  - Added retry 'registry' chain element (#1202)
* Added retry registry client chain element

Signed-off-by: Oleg Solodkov <oleg.solodkov@xored.com>

* Addressed review comments

Signed-off-by: Oleg Solodkov <oleg.solodkov@xored.com>

* Addressed review comments

Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
nsmbot pushed a commit to networkservicemesh/cmd-registry-memory that referenced this pull request Dec 24, 2021
…k@main

PR link: networkservicemesh/sdk#1202

Commit: 29ac91f
Author: Sol-0
Date: 2021-12-24 20:21:38 +0700
Message:
  - Added retry 'registry' chain element (#1202)
* Added retry registry client chain element

Signed-off-by: Oleg Solodkov <oleg.solodkov@xored.com>

* Addressed review comments

Signed-off-by: Oleg Solodkov <oleg.solodkov@xored.com>

* Addressed review comments

Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
nsmbot pushed a commit to networkservicemesh/cmd-nsmgr-proxy that referenced this pull request Dec 24, 2021
…k@main

PR link: networkservicemesh/sdk#1202

Commit: 29ac91f
Author: Sol-0
Date: 2021-12-24 20:21:38 +0700
Message:
  - Added retry 'registry' chain element (#1202)
* Added retry registry client chain element

Signed-off-by: Oleg Solodkov <oleg.solodkov@xored.com>

* Addressed review comments

Signed-off-by: Oleg Solodkov <oleg.solodkov@xored.com>

* Addressed review comments

Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
nsmbot pushed a commit to networkservicemesh/cmd-nsmgr that referenced this pull request Dec 24, 2021
…k@main

PR link: networkservicemesh/sdk#1202

Commit: 29ac91f
Author: Sol-0
Date: 2021-12-24 20:21:38 +0700
Message:
  - Added retry 'registry' chain element (#1202)
* Added retry registry client chain element

Signed-off-by: Oleg Solodkov <oleg.solodkov@xored.com>

* Addressed review comments

Signed-off-by: Oleg Solodkov <oleg.solodkov@xored.com>

* Addressed review comments

Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
nsmbot pushed a commit to networkservicemesh/cmd-nse-vfio that referenced this pull request Dec 24, 2021
…k@main

PR link: networkservicemesh/sdk#1202

Commit: 29ac91f
Author: Sol-0
Date: 2021-12-24 20:21:38 +0700
Message:
  - Added retry 'registry' chain element (#1202)
* Added retry registry client chain element

Signed-off-by: Oleg Solodkov <oleg.solodkov@xored.com>

* Addressed review comments

Signed-off-by: Oleg Solodkov <oleg.solodkov@xored.com>

* Addressed review comments

Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
nsmbot pushed a commit to networkservicemesh/cmd-nsc-init that referenced this pull request Dec 24, 2021
…k@main

PR link: networkservicemesh/sdk#1202

Commit: 29ac91f
Author: Sol-0
Date: 2021-12-24 20:21:38 +0700
Message:
  - Added retry 'registry' chain element (#1202)
* Added retry registry client chain element

Signed-off-by: Oleg Solodkov <oleg.solodkov@xored.com>

* Addressed review comments

Signed-off-by: Oleg Solodkov <oleg.solodkov@xored.com>

* Addressed review comments

Signed-off-by: NSMBot <nsmbot@networkservicmesh.io>
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.

Implement 'retry' registry chain element
2 participants