From a1c8bdb963cc83461d5ccd3ae7b8a024860321bd Mon Sep 17 00:00:00 2001 From: Marcin Rataj Date: Sat, 1 Apr 2023 02:17:13 +0200 Subject: [PATCH] refactor: merge Get and GetRange https://github.com/ipfs/boxo/pull/240#pullrequestreview-1365554977 --- blockstore_proxy.go | 2 +- go.mod | 2 +- go.sum | 4 +-- lib/blockstore_cache.go | 2 +- lib/graph_gateway.go | 63 ++++++++++++---------------------------- test-backend/handlers.go | 29 +++++++++--------- 6 files changed, 39 insertions(+), 63 deletions(-) diff --git a/blockstore_proxy.go b/blockstore_proxy.go index 6b58e96..1a31846 100644 --- a/blockstore_proxy.go +++ b/blockstore_proxy.go @@ -12,7 +12,7 @@ import ( "github.com/ipfs/bifrost-gateway/lib" blockstore "github.com/ipfs/boxo/blockstore" - "github.com/ipfs/go-block-format" + blocks "github.com/ipfs/go-block-format" "github.com/ipfs/go-cid" ) diff --git a/go.mod b/go.mod index 770fbec..e05ca1f 100644 --- a/go.mod +++ b/go.mod @@ -7,7 +7,7 @@ require ( github.com/filecoin-saturn/caboose v0.0.0-20230329203940-7c08345c244d github.com/gogo/protobuf v1.3.2 github.com/hashicorp/golang-lru/v2 v2.0.1 - github.com/ipfs/boxo v0.8.0-rc3.0.20230331021433-e49cc43d95ad + github.com/ipfs/boxo v0.8.0-rc3.0.20230401000028-de9daf5fc542 github.com/ipfs/go-block-format v0.1.2 github.com/ipfs/go-cid v0.4.0 github.com/ipfs/go-ipld-format v0.4.0 diff --git a/go.sum b/go.sum index 0e576a6..78a7ea9 100644 --- a/go.sum +++ b/go.sum @@ -186,8 +186,8 @@ github.com/inconshreveable/mousetrap v1.0.1 h1:U3uMjPSQEBMNp1lFxmllqCPM6P5u/Xq7P github.com/inconshreveable/mousetrap v1.0.1/go.mod h1:vpF70FUmC8bwa3OWnCshd2FqLfsEA9PFc4w1p2J65bw= github.com/ipfs/bbloom v0.0.4 h1:Gi+8EGJ2y5qiD5FbsbpX/TMNcJw8gSqr7eyjHa4Fhvs= github.com/ipfs/bbloom v0.0.4/go.mod h1:cS9YprKXpoZ9lT0n/Mw/a6/aFV6DTjTLYHeA+gyqMG0= -github.com/ipfs/boxo v0.8.0-rc3.0.20230331021433-e49cc43d95ad h1:olq3866OLC+aCW3hImmUtze0Nh8lbFfciTSeq09xfQU= -github.com/ipfs/boxo v0.8.0-rc3.0.20230331021433-e49cc43d95ad/go.mod h1:RIsi4CnTyQ7AUsNn5gXljJYZlQrHBMnJp94p73liFiA= +github.com/ipfs/boxo v0.8.0-rc3.0.20230401000028-de9daf5fc542 h1:2aYk8I/b1NvD2AzsNMhFrCbPjzuarcHVBdGtl0KRST4= +github.com/ipfs/boxo v0.8.0-rc3.0.20230401000028-de9daf5fc542/go.mod h1:RIsi4CnTyQ7AUsNn5gXljJYZlQrHBMnJp94p73liFiA= github.com/ipfs/go-bitfield v1.1.0 h1:fh7FIo8bSwaJEh6DdTWbCeZ1eqOaOkKFI74SCnsWbGA= github.com/ipfs/go-bitfield v1.1.0/go.mod h1:paqf1wjq/D2BBmzfTVFlJQ9IlFOZpg422HL0HqsGWHU= github.com/ipfs/go-block-format v0.0.2/go.mod h1:AWR46JfpcObNfg3ok2JHDUfdiHRgWhJgCQF+KIgOPJY= diff --git a/lib/blockstore_cache.go b/lib/blockstore_cache.go index 7e986a6..a41778c 100644 --- a/lib/blockstore_cache.go +++ b/lib/blockstore_cache.go @@ -19,7 +19,7 @@ import ( const DefaultCacheBlockStoreSize = 1024 -var cacheLog = golog.Logger("cache-blockstore") +var cacheLog = golog.Logger("cache/block") func NewCacheBlockStore(size int) (blockstore.Blockstore, error) { c, err := lru.New2Q[string, []byte](size) diff --git a/lib/graph_gateway.go b/lib/graph_gateway.go index 0df8059..0d6a73e 100644 --- a/lib/graph_gateway.go +++ b/lib/graph_gateway.go @@ -36,7 +36,7 @@ import ( "go.uber.org/multierr" ) -var graphLog = golog.Logger("graph-gateway") +var graphLog = golog.Logger("backend/graph") // type DataCallback = func(resource string, reader io.Reader) error // TODO: Don't use a caboose type, perhaps ask them to use a type alias instead of a type @@ -201,7 +201,7 @@ func registerGraphGatewayMetrics() *GraphGatewayMetrics { Subsystem: "gw_graph_backend", Name: "car_fetch_params", Help: "How many times specific CAR parameter was used during CAR data fetch.", - }, []string{"depth", "bytes"}) + }, []string{"depth", "ranges"}) // we use 'ranges' instead of 'bytes' here because we only caount the number of ranges present prometheus.MustRegister(carParamsMetric) return &GraphGatewayMetrics{ @@ -241,7 +241,7 @@ func (api *GraphGateway) loadRequestIntoSharedBlockstoreAndBlocksGateway(ctx con defer func() { if r := recover(); r != nil { // TODO: move to Debugw? - graphLog.Errorw("Recovered fetcher error", "error", r) + graphLog.Errorw("Recovered fetcher error", "path", path, "error", r) } }() metrics.carFetchAttemptMetric.Inc() @@ -266,11 +266,9 @@ func (api *GraphGateway) loadRequestIntoSharedBlockstoreAndBlocksGateway(ctx con } }) if err != nil { - // TODO: move to debug? - graphLog.Errorw("car Fetch failed", "error", err) + graphLog.Debugw("car Fetch failed", "path", path, "error", err) } if err := carFetchingExch.Close(); err != nil { - // TODO: move to debug? graphLog.Errorw("carFetchingExch.Close()", "error", err) } doneWithFetcher <- struct{}{} @@ -340,36 +338,17 @@ func wrapNodeWithClose[T files.Node](node T, closeFn func()) (T, error) { } } -func (api *GraphGateway) Get(ctx context.Context, path gateway.ImmutablePath) (gateway.ContentPathMetadata, *gateway.GetResponse, error) { - api.metrics.carParamsMetric.With(prometheus.Labels{"depth": "1", "bytes": ""}).Inc() - blkgw, closeFn, err := api.loadRequestIntoSharedBlockstoreAndBlocksGateway(ctx, path.String()+"?format=car&depth=1") - if err != nil { - return gateway.ContentPathMetadata{}, nil, err - } - md, gr, err := blkgw.Get(ctx, path) - if err != nil { - return gateway.ContentPathMetadata{}, nil, err - } - //TODO: interfaces here aren't good enough so we're getting around the problem this way - runtime.SetFinalizer(gr, func(_ *gateway.GetResponse) { closeFn() }) - return md, gr, err -} - -func (api *GraphGateway) GetRange(ctx context.Context, path gateway.ImmutablePath, httpRanges ...gateway.GetRange) (gateway.ContentPathMetadata, files.File, error) { - rangeCount := len(httpRanges) - api.metrics.carParamsMetric.With(prometheus.Labels{ - "depth": "1", - "bytes": fmt.Sprintf("(request ranges: %d)", rangeCount), // we dont pass specific ranges, we want deterministic number of CounterVec labels - }).Inc() +func (api *GraphGateway) Get(ctx context.Context, path gateway.ImmutablePath, byteRanges ...gateway.ByteRange) (gateway.ContentPathMetadata, *gateway.GetResponse, error) { + rangeCount := len(byteRanges) + api.metrics.carParamsMetric.With(prometheus.Labels{"depth": "1", "ranges": strconv.Itoa(rangeCount)}).Inc() carParams := "?format=car&depth=1" - // TODO: refactor this if we ever merge Get and GetRange + // fetch CAR with &bytes= to get minimal set of blocks for the request if rangeCount > 0 { - bytesBuilder := strings.Builder{} bytesBuilder.WriteString("&bytes=") - for i, r := range httpRanges { + for i, r := range byteRanges { bytesBuilder.WriteString(strconv.FormatUint(r.From, 10)) bytesBuilder.WriteString("-") if r.To != nil { @@ -386,19 +365,18 @@ func (api *GraphGateway) GetRange(ctx context.Context, path gateway.ImmutablePat if err != nil { return gateway.ContentPathMetadata{}, nil, err } - md, f, err := blkgw.GetRange(ctx, path) - if err != nil { - return gateway.ContentPathMetadata{}, nil, err - } - f, err = wrapNodeWithClose(f, closeFn) + md, gr, err := blkgw.Get(ctx, path, byteRanges...) if err != nil { return gateway.ContentPathMetadata{}, nil, err } - return md, f, nil + + //TODO: interfaces here aren't good enough so we're getting around the problem this way + runtime.SetFinalizer(gr, func(_ *gateway.GetResponse) { closeFn() }) + return md, gr, nil } func (api *GraphGateway) GetAll(ctx context.Context, path gateway.ImmutablePath) (gateway.ContentPathMetadata, files.Node, error) { - api.metrics.carParamsMetric.With(prometheus.Labels{"depth": "all", "bytes": ""}).Inc() + api.metrics.carParamsMetric.With(prometheus.Labels{"depth": "all", "ranges": "0"}).Inc() blkgw, closeFn, err := api.loadRequestIntoSharedBlockstoreAndBlocksGateway(ctx, path.String()+"?format=car&depth=all") if err != nil { return gateway.ContentPathMetadata{}, nil, err @@ -415,7 +393,7 @@ func (api *GraphGateway) GetAll(ctx context.Context, path gateway.ImmutablePath) } func (api *GraphGateway) GetBlock(ctx context.Context, path gateway.ImmutablePath) (gateway.ContentPathMetadata, files.File, error) { - api.metrics.carParamsMetric.With(prometheus.Labels{"depth": "0", "bytes": ""}).Inc() + api.metrics.carParamsMetric.With(prometheus.Labels{"depth": "0", "ranges": "0"}).Inc() // TODO: if path is `/ipfs/cid`, we should use ?format=raw blkgw, closeFn, err := api.loadRequestIntoSharedBlockstoreAndBlocksGateway(ctx, path.String()+"?format=car&depth=0") if err != nil { @@ -433,10 +411,7 @@ func (api *GraphGateway) GetBlock(ctx context.Context, path gateway.ImmutablePat } func (api *GraphGateway) Head(ctx context.Context, path gateway.ImmutablePath) (gateway.ContentPathMetadata, files.Node, error) { - api.metrics.carParamsMetric.With(prometheus.Labels{ - "depth": "", - "bytes": "0:1023 (Head)", - }).Inc() + api.metrics.carParamsMetric.With(prometheus.Labels{"depth": "", "ranges": "1"}).Inc() blkgw, closeFn, err := api.loadRequestIntoSharedBlockstoreAndBlocksGateway(ctx, path.String()+"?format=car&bytes=0:1023") if err != nil { return gateway.ContentPathMetadata{}, nil, err @@ -453,7 +428,7 @@ func (api *GraphGateway) Head(ctx context.Context, path gateway.ImmutablePath) ( } func (api *GraphGateway) ResolvePath(ctx context.Context, path gateway.ImmutablePath) (gateway.ContentPathMetadata, error) { - api.metrics.carParamsMetric.With(prometheus.Labels{"depth": "0", "bytes": ""}).Inc() + api.metrics.carParamsMetric.With(prometheus.Labels{"depth": "0", "ranges": "0"}).Inc() blkgw, closeFn, err := api.loadRequestIntoSharedBlockstoreAndBlocksGateway(ctx, path.String()+"?format=car&depth=0") if err != nil { return gateway.ContentPathMetadata{}, err @@ -463,7 +438,7 @@ func (api *GraphGateway) ResolvePath(ctx context.Context, path gateway.Immutable } func (api *GraphGateway) GetCAR(ctx context.Context, path gateway.ImmutablePath) (gateway.ContentPathMetadata, io.ReadCloser, <-chan error, error) { - api.metrics.carParamsMetric.With(prometheus.Labels{"depth": "", "bytes": ""}).Inc() + api.metrics.carParamsMetric.With(prometheus.Labels{"depth": "", "ranges": "0"}).Inc() blkgw, closeFn, err := api.loadRequestIntoSharedBlockstoreAndBlocksGateway(ctx, path.String()+"?format=car") if err != nil { return gateway.ContentPathMetadata{}, nil, nil, err diff --git a/test-backend/handlers.go b/test-backend/handlers.go index 23c5d85..ad81d40 100644 --- a/test-backend/handlers.go +++ b/test-backend/handlers.go @@ -4,6 +4,15 @@ import ( "bytes" "context" "fmt" + "io" + "net/http" + "net/url" + "runtime/debug" + "strconv" + "strings" + "sync" + "time" + "github.com/ipfs/boxo/fetcher" "github.com/ipfs/boxo/files" "github.com/ipfs/boxo/gateway" @@ -13,7 +22,7 @@ import ( unixfile "github.com/ipfs/boxo/ipld/unixfs/file" "github.com/ipfs/boxo/path" "github.com/ipfs/boxo/path/resolver" - "github.com/ipfs/go-block-format" + blocks "github.com/ipfs/go-block-format" format "github.com/ipfs/go-ipld-format" "github.com/ipfs/go-unixfsnode" dagpb "github.com/ipld/go-codec-dagpb" @@ -23,14 +32,6 @@ import ( "github.com/ipld/go-ipld-prime/schema" "github.com/ipld/go-ipld-prime/traversal" "github.com/ipld/go-ipld-prime/traversal/selector" - "io" - "net/http" - "net/url" - "runtime/debug" - "strconv" - "strings" - "sync" - "time" "github.com/prometheus/client_golang/prometheus" "github.com/prometheus/client_golang/prometheus/promhttp" @@ -189,9 +190,9 @@ func simpleSelectorToCar(ctx context.Context, bsrv blockservice.BlockService, p if hasDepth && !(depthStr == "0" || depthStr == "1" || depthStr == "all") { return nil, fmt.Errorf("depth type: %s not supported", depthStr) } - var getRange *gateway.GetRange + var getRange *gateway.ByteRange if hasRange { - getRange, err = rangeStrToGetRange(rangeStr) + getRange, err = rangeStrToByteRange(rangeStr) if err != nil { return nil, err } @@ -478,7 +479,7 @@ func blockOpener(ctx context.Context, ng format.NodeGetter) ipld.BlockReadOpener var _ fetcher.Fetcher = (*nodeGetterFetcherSingleUseFactory)(nil) var _ fetcher.Factory = (*nodeGetterFetcherSingleUseFactory)(nil) -func rangeStrToGetRange(rangeStr string) (*gateway.GetRange, error) { +func rangeStrToByteRange(rangeStr string) (*gateway.ByteRange, error) { rangeElems := strings.Split(rangeStr, ":") if len(rangeElems) > 2 { return nil, fmt.Errorf("invalid range") @@ -489,7 +490,7 @@ func rangeStrToGetRange(rangeStr string) (*gateway.GetRange, error) { } if rangeElems[1] == "*" { - return &gateway.GetRange{ + return &gateway.ByteRange{ From: first, To: nil, }, nil @@ -509,7 +510,7 @@ func rangeStrToGetRange(rangeStr string) (*gateway.GetRange, error) { return nil, fmt.Errorf("invalid range") } - return &gateway.GetRange{ + return &gateway.ByteRange{ From: first, To: &second, }, nil