From 704add47a87c7933007df979cf71c5b47333893c Mon Sep 17 00:00:00 2001 From: Prince Kumar Date: Mon, 22 Aug 2022 12:55:51 +0530 Subject: [PATCH 1/8] Adding API to Create Storage Client Handle --- internal/gcsx/storage_handle.go | 68 ++++++++++++++++++++++++++++ internal/gcsx/storage_handle_test.go | 56 +++++++++++++++++++++++ 2 files changed, 124 insertions(+) create mode 100644 internal/gcsx/storage_handle.go create mode 100644 internal/gcsx/storage_handle_test.go diff --git a/internal/gcsx/storage_handle.go b/internal/gcsx/storage_handle.go new file mode 100644 index 0000000000..e69e8b4886 --- /dev/null +++ b/internal/gcsx/storage_handle.go @@ -0,0 +1,68 @@ +package gcsx + +import ( + "crypto/tls" + "fmt" + "net/http" + "time" + + "cloud.google.com/go/storage" + "golang.org/x/net/context" + "golang.org/x/oauth2" + "google.golang.org/api/option" +) + +type storageHandle struct { + client *storage.Client +} + +type storageClientConfig struct { + disableHttp2 bool + maxConnsPerHost int + maxIdleConnsPerHost int + tokenSrc oauth2.TokenSource +} + +// GetStorageClientHandle returns the handle of Go storage client containing +// customized http client. We can configure the http client using the +// storageClientConfig parameter. +func GetStorageClientHandle(ctx context.Context, + sc storageClientConfig) (sh *storageHandle, err error) { + var storageClient *storage.Client = nil + + var tr *http.Transport = nil + // Choosing between HTTP1 and HTTP2. HTTP2 is more performant. + if sc.disableHttp2 { + tr = &http.Transport{ + MaxConnsPerHost: sc.maxConnsPerHost, + MaxIdleConnsPerHost: sc.maxIdleConnsPerHost, + // This disables HTTP/2 in transport. + TLSNextProto: make( + map[string]func(string, *tls.Conn) http.RoundTripper, + ), + } + } else { + tr = &http.Transport{ + DisableKeepAlives: true, + MaxConnsPerHost: sc.maxConnsPerHost, // not affect the performance for http2 + ForceAttemptHTTP2: true, + } + } + + // Custom http Client for Go Client. + httpClient := &http.Client{Transport: &oauth2.Transport{ + Base: tr, + Source: sc.tokenSrc, + }, + Timeout: 800 * time.Millisecond, + } + + // check retry strategy should be enabled here. + storageClient, err = storage.NewClient(ctx, option.WithHTTPClient(httpClient)) + if err != nil { + err = fmt.Errorf("go storage client creation: %v", err) + } + sh = &storageHandle{storageClient} + + return +} diff --git a/internal/gcsx/storage_handle_test.go b/internal/gcsx/storage_handle_test.go new file mode 100644 index 0000000000..f591448f61 --- /dev/null +++ b/internal/gcsx/storage_handle_test.go @@ -0,0 +1,56 @@ +package gcsx + +import ( + "context" + "testing" + + "golang.org/x/oauth2" +) + +func TestNewStorageHandleHttp2Disabled(t *testing.T) { + sc := storageClientConfig{disableHttp2: true, + maxConnsPerHost: 10, + maxIdleConnsPerHost: 100, + tokenSrc: oauth2.StaticTokenSource(&oauth2.Token{})} + + handleCreated, err := GetStorageClientHandle(context.Background(), sc) + + if err != nil { + t.Errorf("Handle creation failure") + } + if handleCreated == nil { + t.Errorf("Storage handle is null") + } +} + +func TestNewStorageHandleHttp2Enabled(t *testing.T) { + sc := storageClientConfig{disableHttp2: false, + maxConnsPerHost: 10, + maxIdleConnsPerHost: 100, + tokenSrc: oauth2.StaticTokenSource(&oauth2.Token{})} + + handleCreated, err := GetStorageClientHandle(context.Background(), sc) + + if err != nil { + t.Errorf("Handle creation failure") + } + if handleCreated == nil { + t.Errorf("Storage handle is null") + } +} + +func TestNewStorageHandleWithZeroMaxConnsPerHost(t *testing.T) { + sc := storageClientConfig{disableHttp2: true, + maxConnsPerHost: 0, + maxIdleConnsPerHost: 100, + tokenSrc: oauth2.StaticTokenSource(&oauth2.Token{})} + + handleCreated, err := GetStorageClientHandle(context.Background(), sc) + + if err != nil { + t.Errorf("Handle creation failure") + } + if handleCreated == nil { + t.Errorf("Storage handle is null") + } +} From bd00109d35a63e3f4701c6af65ed80b0d887e61e Mon Sep 17 00:00:00 2001 From: Prince Kumar Date: Mon, 22 Aug 2022 13:03:45 +0530 Subject: [PATCH 2/8] Fixing linting issue --- internal/gcsx/storage_handle.go | 4 ++-- internal/gcsx/storage_handle_test.go | 9 +++++++++ 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/internal/gcsx/storage_handle.go b/internal/gcsx/storage_handle.go index e69e8b4886..629c705cfc 100644 --- a/internal/gcsx/storage_handle.go +++ b/internal/gcsx/storage_handle.go @@ -28,9 +28,9 @@ type storageClientConfig struct { // storageClientConfig parameter. func GetStorageClientHandle(ctx context.Context, sc storageClientConfig) (sh *storageHandle, err error) { - var storageClient *storage.Client = nil + var storageClient *storage.Client - var tr *http.Transport = nil + var tr *http.Transport // Choosing between HTTP1 and HTTP2. HTTP2 is more performant. if sc.disableHttp2 { tr = &http.Transport{ diff --git a/internal/gcsx/storage_handle_test.go b/internal/gcsx/storage_handle_test.go index f591448f61..b0678fb35e 100644 --- a/internal/gcsx/storage_handle_test.go +++ b/internal/gcsx/storage_handle_test.go @@ -21,6 +21,9 @@ func TestNewStorageHandleHttp2Disabled(t *testing.T) { if handleCreated == nil { t.Errorf("Storage handle is null") } + if handleCreated.client == nil { + t.Errorf("Storage client handle is null") + } } func TestNewStorageHandleHttp2Enabled(t *testing.T) { @@ -37,6 +40,9 @@ func TestNewStorageHandleHttp2Enabled(t *testing.T) { if handleCreated == nil { t.Errorf("Storage handle is null") } + if handleCreated.client == nil { + t.Errorf("Storage client handle is null") + } } func TestNewStorageHandleWithZeroMaxConnsPerHost(t *testing.T) { @@ -53,4 +59,7 @@ func TestNewStorageHandleWithZeroMaxConnsPerHost(t *testing.T) { if handleCreated == nil { t.Errorf("Storage handle is null") } + if handleCreated.client == nil { + t.Errorf("Storage client handle is null") + } } From 7b1cda31d3d37f11b2c8be9f8d7949ef8d6f939d Mon Sep 17 00:00:00 2001 From: Prince Kumar Date: Mon, 22 Aug 2022 16:55:02 +0530 Subject: [PATCH 3/8] Fixing linting issue in storage client test --- internal/gcsx/storage_handle_test.go | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/internal/gcsx/storage_handle_test.go b/internal/gcsx/storage_handle_test.go index b0678fb35e..f96f105987 100644 --- a/internal/gcsx/storage_handle_test.go +++ b/internal/gcsx/storage_handle_test.go @@ -18,11 +18,11 @@ func TestNewStorageHandleHttp2Disabled(t *testing.T) { if err != nil { t.Errorf("Handle creation failure") } - if handleCreated == nil { - t.Errorf("Storage handle is null") + if nil == handleCreated { + t.Fatalf("Storage handle is null") } - if handleCreated.client == nil { - t.Errorf("Storage client handle is null") + if nil == handleCreated.client { + t.Fatalf("Storage client handle is null") } } @@ -37,11 +37,11 @@ func TestNewStorageHandleHttp2Enabled(t *testing.T) { if err != nil { t.Errorf("Handle creation failure") } - if handleCreated == nil { - t.Errorf("Storage handle is null") + if nil == handleCreated { + t.Fatalf("Storage handle is null") } - if handleCreated.client == nil { - t.Errorf("Storage client handle is null") + if nil == handleCreated.client { + t.Fatalf("Storage client handle is null") } } @@ -56,10 +56,10 @@ func TestNewStorageHandleWithZeroMaxConnsPerHost(t *testing.T) { if err != nil { t.Errorf("Handle creation failure") } - if handleCreated == nil { - t.Errorf("Storage handle is null") + if nil == handleCreated { + t.Fatalf("Storage handle is null") } - if handleCreated.client == nil { - t.Errorf("Storage client handle is null") + if nil == handleCreated.client { + t.Fatalf("Storage client handle is null") } } From c15e46822ca0a9653b489fc56852d0188d5e45a2 Mon Sep 17 00:00:00 2001 From: Prince Kumar Date: Tue, 23 Aug 2022 08:32:45 +0530 Subject: [PATCH 4/8] Incorporating review comments --- internal/gcsx/storage_handle.go | 40 ++++++++++++++-------------- internal/gcsx/storage_handle_test.go | 6 ++--- 2 files changed, 23 insertions(+), 23 deletions(-) diff --git a/internal/gcsx/storage_handle.go b/internal/gcsx/storage_handle.go index 629c705cfc..92406635b2 100644 --- a/internal/gcsx/storage_handle.go +++ b/internal/gcsx/storage_handle.go @@ -12,12 +12,12 @@ import ( "google.golang.org/api/option" ) -type storageHandle struct { +type storageClient struct { client *storage.Client } type storageClientConfig struct { - disableHttp2 bool + disableHTTP2 bool maxConnsPerHost int maxIdleConnsPerHost int tokenSrc oauth2.TokenSource @@ -27,42 +27,42 @@ type storageClientConfig struct { // customized http client. We can configure the http client using the // storageClientConfig parameter. func GetStorageClientHandle(ctx context.Context, - sc storageClientConfig) (sh *storageHandle, err error) { - var storageClient *storage.Client - - var tr *http.Transport - // Choosing between HTTP1 and HTTP2. HTTP2 is more performant. - if sc.disableHttp2 { - tr = &http.Transport{ - MaxConnsPerHost: sc.maxConnsPerHost, - MaxIdleConnsPerHost: sc.maxIdleConnsPerHost, + scConfig storageClientConfig) (sh *storageClient, err error) { + var transport *http.Transport + // Disabling the http2 makes the client more performant. + if scConfig.disableHTTP2 { + transport = &http.Transport{ + MaxConnsPerHost: scConfig.maxConnsPerHost, + MaxIdleConnsPerHost: scConfig.maxIdleConnsPerHost, // This disables HTTP/2 in transport. TLSNextProto: make( map[string]func(string, *tls.Conn) http.RoundTripper, ), } } else { - tr = &http.Transport{ + // For http2, change in MaxConnsPerHost doesn't affect the performance. + transport = &http.Transport{ DisableKeepAlives: true, - MaxConnsPerHost: sc.maxConnsPerHost, // not affect the performance for http2 + MaxConnsPerHost: scConfig.maxConnsPerHost, ForceAttemptHTTP2: true, } } - // Custom http Client for Go Client. + // Custom http client for Go Client. httpClient := &http.Client{Transport: &oauth2.Transport{ - Base: tr, - Source: sc.tokenSrc, + Base: transport, + Source: scConfig.tokenSrc, }, Timeout: 800 * time.Millisecond, } - // check retry strategy should be enabled here. - storageClient, err = storage.NewClient(ctx, option.WithHTTPClient(httpClient)) + var sc *storage.Client + sc, err = storage.NewClient(ctx, option.WithHTTPClient(httpClient)) if err != nil { - err = fmt.Errorf("go storage client creation: %v", err) + err = fmt.Errorf("go storage client creation: %w", err) + return } - sh = &storageHandle{storageClient} + sh = &storageClient{sc} return } diff --git a/internal/gcsx/storage_handle_test.go b/internal/gcsx/storage_handle_test.go index f96f105987..557c317b8b 100644 --- a/internal/gcsx/storage_handle_test.go +++ b/internal/gcsx/storage_handle_test.go @@ -8,7 +8,7 @@ import ( ) func TestNewStorageHandleHttp2Disabled(t *testing.T) { - sc := storageClientConfig{disableHttp2: true, + sc := storageClientConfig{disableHTTP2: true, maxConnsPerHost: 10, maxIdleConnsPerHost: 100, tokenSrc: oauth2.StaticTokenSource(&oauth2.Token{})} @@ -27,7 +27,7 @@ func TestNewStorageHandleHttp2Disabled(t *testing.T) { } func TestNewStorageHandleHttp2Enabled(t *testing.T) { - sc := storageClientConfig{disableHttp2: false, + sc := storageClientConfig{disableHTTP2: false, maxConnsPerHost: 10, maxIdleConnsPerHost: 100, tokenSrc: oauth2.StaticTokenSource(&oauth2.Token{})} @@ -46,7 +46,7 @@ func TestNewStorageHandleHttp2Enabled(t *testing.T) { } func TestNewStorageHandleWithZeroMaxConnsPerHost(t *testing.T) { - sc := storageClientConfig{disableHttp2: true, + sc := storageClientConfig{disableHTTP2: true, maxConnsPerHost: 0, maxIdleConnsPerHost: 100, tokenSrc: oauth2.StaticTokenSource(&oauth2.Token{})} From 1eabee9f90f8230f6a5833482db34d6107da05c3 Mon Sep 17 00:00:00 2001 From: Prince Kumar Date: Tue, 23 Aug 2022 10:05:37 +0530 Subject: [PATCH 5/8] Resolving linting issue --- internal/gcsx/storage_handle.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/gcsx/storage_handle.go b/internal/gcsx/storage_handle.go index 92406635b2..cade8f64a1 100644 --- a/internal/gcsx/storage_handle.go +++ b/internal/gcsx/storage_handle.go @@ -27,7 +27,7 @@ type storageClientConfig struct { // customized http client. We can configure the http client using the // storageClientConfig parameter. func GetStorageClientHandle(ctx context.Context, - scConfig storageClientConfig) (sh *storageClient, err error) { + scConfig storageClientConfig) (sh *storageClient, err error) { var transport *http.Transport // Disabling the http2 makes the client more performant. if scConfig.disableHTTP2 { From 8538655eb8ad37f31a3a1e7857f39e9924b2b529 Mon Sep 17 00:00:00 2001 From: Prince Kumar Date: Tue, 23 Aug 2022 12:50:36 +0530 Subject: [PATCH 6/8] Review comments: refactoring --- internal/gcsx/storage_handle.go | 28 +++++++------- internal/gcsx/storage_handle_test.go | 56 ++++++++++++---------------- 2 files changed, 39 insertions(+), 45 deletions(-) diff --git a/internal/gcsx/storage_handle.go b/internal/gcsx/storage_handle.go index cade8f64a1..07d26423cf 100644 --- a/internal/gcsx/storage_handle.go +++ b/internal/gcsx/storage_handle.go @@ -21,19 +21,20 @@ type storageClientConfig struct { maxConnsPerHost int maxIdleConnsPerHost int tokenSrc oauth2.TokenSource + timeOut time.Duration } -// GetStorageClientHandle returns the handle of Go storage client containing +// NewStorageHandle returns the handle of Go storage client containing // customized http client. We can configure the http client using the // storageClientConfig parameter. -func GetStorageClientHandle(ctx context.Context, - scConfig storageClientConfig) (sh *storageClient, err error) { +func NewStorageHandle(ctx context.Context, + clientConfig storageClientConfig) (sh *storageClient, err error) { var transport *http.Transport // Disabling the http2 makes the client more performant. - if scConfig.disableHTTP2 { + if clientConfig.disableHTTP2 { transport = &http.Transport{ - MaxConnsPerHost: scConfig.maxConnsPerHost, - MaxIdleConnsPerHost: scConfig.maxIdleConnsPerHost, + MaxConnsPerHost: clientConfig.maxConnsPerHost, + MaxIdleConnsPerHost: clientConfig.maxIdleConnsPerHost, // This disables HTTP/2 in transport. TLSNextProto: make( map[string]func(string, *tls.Conn) http.RoundTripper, @@ -43,23 +44,24 @@ func GetStorageClientHandle(ctx context.Context, // For http2, change in MaxConnsPerHost doesn't affect the performance. transport = &http.Transport{ DisableKeepAlives: true, - MaxConnsPerHost: scConfig.maxConnsPerHost, + MaxConnsPerHost: clientConfig.maxConnsPerHost, ForceAttemptHTTP2: true, } } // Custom http client for Go Client. - httpClient := &http.Client{Transport: &oauth2.Transport{ - Base: transport, - Source: scConfig.tokenSrc, - }, - Timeout: 800 * time.Millisecond, + httpClient := &http.Client{ + Transport: &oauth2.Transport{ + Base: transport, + Source: clientConfig.tokenSrc, + }, + Timeout: clientConfig.timeOut, } var sc *storage.Client sc, err = storage.NewClient(ctx, option.WithHTTPClient(httpClient)) if err != nil { - err = fmt.Errorf("go storage client creation: %w", err) + err = fmt.Errorf("go storage client creation failed: %w", err) return } diff --git a/internal/gcsx/storage_handle_test.go b/internal/gcsx/storage_handle_test.go index 557c317b8b..66b757f46f 100644 --- a/internal/gcsx/storage_handle_test.go +++ b/internal/gcsx/storage_handle_test.go @@ -3,63 +3,55 @@ package gcsx import ( "context" "testing" + "time" "golang.org/x/oauth2" ) -func TestNewStorageHandleHttp2Disabled(t *testing.T) { - sc := storageClientConfig{disableHTTP2: true, - maxConnsPerHost: 10, - maxIdleConnsPerHost: 100, - tokenSrc: oauth2.StaticTokenSource(&oauth2.Token{})} - - handleCreated, err := GetStorageClientHandle(context.Background(), sc) - +func verifyStorageHandle(t *testing.T, handle *storageClient, err error) { if err != nil { t.Errorf("Handle creation failure") } - if nil == handleCreated { + if nil == handle { t.Fatalf("Storage handle is null") } - if nil == handleCreated.client { + if nil == handle.client { t.Fatalf("Storage client handle is null") } } +func TestNewStorageHandleHttp2Disabled(t *testing.T) { + sc := storageClientConfig{disableHTTP2: true, + maxConnsPerHost: 10, + maxIdleConnsPerHost: 100, + tokenSrc: oauth2.StaticTokenSource(&oauth2.Token{}), + timeOut: 800 * time.Millisecond} + + handleCreated, err := NewStorageHandle(context.Background(), sc) + + verifyStorageHandle(t, handleCreated, err) +} + func TestNewStorageHandleHttp2Enabled(t *testing.T) { sc := storageClientConfig{disableHTTP2: false, maxConnsPerHost: 10, maxIdleConnsPerHost: 100, - tokenSrc: oauth2.StaticTokenSource(&oauth2.Token{})} + tokenSrc: oauth2.StaticTokenSource(&oauth2.Token{}), + timeOut: 800 * time.Millisecond} - handleCreated, err := GetStorageClientHandle(context.Background(), sc) + handleCreated, err := NewStorageHandle(context.Background(), sc) - if err != nil { - t.Errorf("Handle creation failure") - } - if nil == handleCreated { - t.Fatalf("Storage handle is null") - } - if nil == handleCreated.client { - t.Fatalf("Storage client handle is null") - } + verifyStorageHandle(t, handleCreated, err) } func TestNewStorageHandleWithZeroMaxConnsPerHost(t *testing.T) { sc := storageClientConfig{disableHTTP2: true, maxConnsPerHost: 0, maxIdleConnsPerHost: 100, - tokenSrc: oauth2.StaticTokenSource(&oauth2.Token{})} + tokenSrc: oauth2.StaticTokenSource(&oauth2.Token{}), + timeOut: 800 * time.Millisecond} - handleCreated, err := GetStorageClientHandle(context.Background(), sc) + handleCreated, err := NewStorageHandle(context.Background(), sc) - if err != nil { - t.Errorf("Handle creation failure") - } - if nil == handleCreated { - t.Fatalf("Storage handle is null") - } - if nil == handleCreated.client { - t.Fatalf("Storage client handle is null") - } + verifyStorageHandle(t, handleCreated, err) } From 290f806fa8e5a398ea5df25f5b58580430b22ed1 Mon Sep 17 00:00:00 2001 From: Prince Kumar Date: Tue, 23 Aug 2022 13:06:12 +0530 Subject: [PATCH 7/8] Review comments: refactoring --- internal/gcsx/storage_handle_test.go | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/internal/gcsx/storage_handle_test.go b/internal/gcsx/storage_handle_test.go index 66b757f46f..f44e13d4af 100644 --- a/internal/gcsx/storage_handle_test.go +++ b/internal/gcsx/storage_handle_test.go @@ -8,14 +8,16 @@ import ( "golang.org/x/oauth2" ) -func verifyStorageHandle(t *testing.T, handle *storageClient, err error) { +func invokeAndVerifyStorageHandle(t *testing.T, sc storageClientConfig) { + handleCreated, err := NewStorageHandle(context.Background(), sc) + if err != nil { t.Errorf("Handle creation failure") } - if nil == handle { + if nil == handleCreated { t.Fatalf("Storage handle is null") } - if nil == handle.client { + if nil == handleCreated.client { t.Fatalf("Storage client handle is null") } } @@ -27,9 +29,7 @@ func TestNewStorageHandleHttp2Disabled(t *testing.T) { tokenSrc: oauth2.StaticTokenSource(&oauth2.Token{}), timeOut: 800 * time.Millisecond} - handleCreated, err := NewStorageHandle(context.Background(), sc) - - verifyStorageHandle(t, handleCreated, err) + invokeAndVerifyStorageHandle(t, sc) } func TestNewStorageHandleHttp2Enabled(t *testing.T) { @@ -39,9 +39,7 @@ func TestNewStorageHandleHttp2Enabled(t *testing.T) { tokenSrc: oauth2.StaticTokenSource(&oauth2.Token{}), timeOut: 800 * time.Millisecond} - handleCreated, err := NewStorageHandle(context.Background(), sc) - - verifyStorageHandle(t, handleCreated, err) + invokeAndVerifyStorageHandle(t, sc) } func TestNewStorageHandleWithZeroMaxConnsPerHost(t *testing.T) { @@ -51,7 +49,5 @@ func TestNewStorageHandleWithZeroMaxConnsPerHost(t *testing.T) { tokenSrc: oauth2.StaticTokenSource(&oauth2.Token{}), timeOut: 800 * time.Millisecond} - handleCreated, err := NewStorageHandle(context.Background(), sc) - - verifyStorageHandle(t, handleCreated, err) + invokeAndVerifyStorageHandle(t, sc) } From c945861ac21d6d69057973d06f79470717dead8c Mon Sep 17 00:00:00 2001 From: Prince Kumar Date: Tue, 23 Aug 2022 13:49:01 +0530 Subject: [PATCH 8/8] Moving the file to a new package: storage --- internal/{gcsx => storage}/storage_handle.go | 2 +- internal/{gcsx => storage}/storage_handle_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) rename internal/{gcsx => storage}/storage_handle.go (99%) rename internal/{gcsx => storage}/storage_handle_test.go (98%) diff --git a/internal/gcsx/storage_handle.go b/internal/storage/storage_handle.go similarity index 99% rename from internal/gcsx/storage_handle.go rename to internal/storage/storage_handle.go index 07d26423cf..d12d05dd6e 100644 --- a/internal/gcsx/storage_handle.go +++ b/internal/storage/storage_handle.go @@ -1,4 +1,4 @@ -package gcsx +package storage import ( "crypto/tls" diff --git a/internal/gcsx/storage_handle_test.go b/internal/storage/storage_handle_test.go similarity index 98% rename from internal/gcsx/storage_handle_test.go rename to internal/storage/storage_handle_test.go index f44e13d4af..7e3d4bceaf 100644 --- a/internal/gcsx/storage_handle_test.go +++ b/internal/storage/storage_handle_test.go @@ -1,4 +1,4 @@ -package gcsx +package storage import ( "context"