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

New advanced client #82

Merged
merged 8 commits into from
Mar 15, 2019
Merged

New advanced client #82

merged 8 commits into from
Mar 15, 2019

Conversation

aalda
Copy link
Contributor

@aalda aalda commented Mar 15, 2019

No description provided.

@aalda aalda changed the title New calient New advance client Mar 15, 2019
@aalda aalda requested review from gdiazlo and iknite March 15, 2019 11:08
@aalda aalda changed the title New advance client New advanced client Mar 15, 2019
Make client aware of the state of all cluster nodes.
Add topology discovery requests.
Add healthcheks.
Add primary/secondary request routing.
Add round-robin endpoint selector for reads.
Add pluggable request retriers.
Add pluggable backoff retrying policies.
Add pluggable read preference option.
Refactor client configuration.
@@ -114,7 +114,7 @@ func TestQueryConsistencyProof(t *testing.T) {
balloon, err := NewBalloon(store, hashing.NewFakeXorHasher)
require.NoError(t, err)

for j := 0; j <= int(c.addtions); j++ {
for j := 0; j <= int(c.additions); j++ {
Copy link
Contributor

Choose a reason for hiding this comment

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

thanks... 🤕

// The number of retries is set to 0.
func NewSimpleHTTPClient(httpClient *http.Client, urls []string) (*HTTPClient, error) {

// defaultTransport := http.DefaultTransport.(*http.Transport)
Copy link
Contributor

Choose a reason for hiding this comment

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

Cleanup

}

// DefaultConfig creates a Config structures with default values.
func DefaultConfig() *Config {
return &Config{
Endpoints: []string{"127.0.0.1:8800"},
Copy link
Contributor

Choose a reason for hiding this comment

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

Everything should be defaulted in the same place, so maybe we need to create DefaultEndpoints and DefaultApiKeys?

Copy link
Contributor Author

@aalda aalda Mar 15, 2019

Choose a reason for hiding this comment

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

The client's constructor also uses the default values. Note that, now, you can construct the client with a conf or with options.

Dial: (&net.Dialer{
Timeout: conf.DialTimeout,
}).Dial,
Proxy: defaultTransport.Proxy,
Copy link
Contributor

Choose a reason for hiding this comment

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

defaultTransport is already a copy of http.DefaultTansport (since the struct is complex) so instead of reinstantiate a new http.Transport we can configure it.

... and all the defaults remain (and also futurible new params)

Copy link
Contributor

@iknite iknite Mar 15, 2019

Choose a reason for hiding this comment

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

something like...

	transport := http.DefaultTransport.(*http.Transport)
        transport.Dial = (&net.Dialer{
					Timeout: conf.DialTimeout,
				}).Dial

		options = append(options, SetHttpClient(&http.Client{
			Timeout: conf.Timeout,
			Transport: transport, })
 

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm going to get rid of most of these transport options in a future refactoring. Anyway, thanks for the tip...

"github.com/bbva/qed/log"
)

var (
Copy link
Contributor

Choose a reason for hiding this comment

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

const?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, it should be a const.

@@ -68,12 +70,22 @@ func NewMonitor(conf Config) (*Monitor, error) {
// Metrics
metrics.QedMonitorInstancesCount.Inc()

// QED client
transport := http.DefaultTransport.(*http.Transport)
Copy link
Contributor

Choose a reason for hiding this comment

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

this 😄 is what I was meaning in client/options.go:53 comment

@iknite
Copy link
Contributor

iknite commented Mar 15, 2019

Also NewHTTPClientFromConfig seems overkill since it's only used twice

tests/riot.go
279:	client, err := client.NewHTTPClientFromConfig(cConf)

cmd/client.go
69:		client, err := client.NewHTTPClientFromConfig(clientCtx.config)

@aalda
Copy link
Contributor Author

aalda commented Mar 15, 2019

Also NewHTTPClientFromConfig seems overkill since it's only used twice

tests/riot.go
279:	client, err := client.NewHTTPClientFromConfig(cConf)

cmd/client.go
69:		client, err := client.NewHTTPClientFromConfig(clientCtx.config)

The client is also intended to be used outside QED project. I prefer to make the client's construction more flexible.


// NextReadendpoint returns the next available endpoint to query
// in a round-robin manner, or ErrNoEndpoint
func (t *topology) NextReadEndpoint(pref ReadPref) (*endpoint, error) {
Copy link
Contributor

@gdiazlo gdiazlo Mar 15, 2019

Choose a reason for hiding this comment

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

This method should probably be simplified on a future revision of the client.

Copy link
Contributor

@gdiazlo gdiazlo left a comment

Choose a reason for hiding this comment

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

LGETM, we will need to clean / simplify things in future revisions

@gdiazlo gdiazlo merged commit f594e71 into BBVA:master Mar 15, 2019
@aalda aalda deleted the new_client branch March 25, 2019 14:28
suizman pushed a commit to jllucas/qed that referenced this pull request Jun 4, 2019
New advanced client

Former-commit-id: f594e71
suizman pushed a commit that referenced this pull request Sep 30, 2019
New advanced client

Former-commit-id: 6693930 [formerly f594e71]
Former-commit-id: 595c6d5
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.

3 participants