Add "get metarange", "get range" API endpoints #1465

arielshaqed · 2021-02-16T07:42:08Z

Returns data (currently just the address of the object) by ID.

codecov-io · 2021-02-16T07:53:09Z

Codecov Report

Merging #1465 (1873a4c) into master (7d63bc5) will decrease coverage by 0.21%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master    #1465      +/-   ##
==========================================
- Coverage   39.37%   39.15%   -0.22%     
==========================================
  Files         161      161              
  Lines       12893    12956      +63     
==========================================
- Hits         5076     5073       -3     
- Misses       7086     7151      +65     
- Partials      731      732       +1

Impacted Files	Coverage Δ
api/controller.go	`35.85% <0.00%> (-0.69%)`	⬇️
catalog/entry_catalog.go	`12.88% <0.00%> (-0.08%)`	⬇️
catalog/rocks_cataloger.go	`24.11% <0.00%> (-0.12%)`	⬇️
graveler/committed/manager.go	`0.00% <0.00%> (ø)`
graveler/committed/meta_range_manager.go	`0.00% <0.00%> (ø)`
graveler/committed/range.go	`87.50% <ø> (ø)`
graveler/committed/range_manager.go	`100.00% <ø> (ø)`
graveler/graveler.go	`30.51% <0.00%> (-0.34%)`	⬇️
graveler/sstable/range_manager.go	`30.88% <0.00%> (-0.47%)`	⬇️
pyramid/tier_fs.go	`56.76% <0.00%> (-2.01%)`	⬇️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7d63bc5...e996175. Read the comment docs.

nopcoder

Looks great, with minor comments/suggestions and a question down at the end before you commit.

nopcoder · 2021-02-17T06:00:34Z

graveler/graveler.go

+type Plumbing interface {
+	// GetMetarange returns information where metarangeID is stored.
+	GetMetaRange(ctx context.Context, repositoryID RepositoryID, metaRangeID MetaRangeID) (MetaRangeData, error)
+	// GetRange returns information where rangeID is stored.


Consider calling it xxxStats returning xxxInfo
ex: MetaRangeStat -> MetaRangeInfo

Not sure I understand the first part, it's not a "stat". But happy to change "Data" to "Info" (it's pretty much to avoid having another "MetaRange" type...).

nopcoder · 2021-02-17T06:10:29Z

api/controller.go

+	return metadata.GetMetaRangeHandlerFunc(func(params metadata.GetMetaRangeParams, user *models.User) middleware.Responder {
+		deps, err := c.setupRequest(user, params.HTTPRequest, []permissions.Permission{
+			{
+				// TODO(oz): May want tighter permissions here.


Remember to remove the todo after verifying

@ozkatz are we good with these?

After f2fing, changed to require both list objects and read repo: it gives a cross between the 2. E.g. with (just) read permission on underlying storage it gives list objects (and without it gives size of branch), which is list-objects-like. But it also gives the storage namespace for the repo, which is read-repo-like. So best to require both.

nopcoder · 2021-02-17T06:11:10Z

api/controller.go

+	return metadata.GetRangeHandlerFunc(func(params metadata.GetRangeParams, user *models.User) middleware.Responder {
+		deps, err := c.setupRequest(user, params.HTTPRequest, []permissions.Permission{
+			{
+				// TODO(oz): May want tighter permissions here.


Remember to remove the todo after verifying

swagger.yml

arielshaqed

Thanks!

Keeping the Location header, changing the names from "...Data" to "...Info".

arielshaqed · 2021-02-17T07:05:21Z

api/controller.go

+	return metadata.GetMetaRangeHandlerFunc(func(params metadata.GetMetaRangeParams, user *models.User) middleware.Responder {
+		deps, err := c.setupRequest(user, params.HTTPRequest, []permissions.Permission{
+			{
+				// TODO(oz): May want tighter permissions here.


@ozkatz are we good with these?

arielshaqed · 2021-02-17T07:15:00Z

graveler/graveler.go

+type Plumbing interface {
+	// GetMetarange returns information where metarangeID is stored.
+	GetMetaRange(ctx context.Context, repositoryID RepositoryID, metaRangeID MetaRangeID) (MetaRangeData, error)
+	// GetRange returns information where rangeID is stored.


Not sure I understand the first part, it's not a "stat". But happy to change "Data" to "Info" (it's pretty much to avoid having another "MetaRange" type...).

swagger.yml

arielshaqed · 2021-02-17T09:48:09Z

@ozkatz to look at permissions for these actions, please.

arielshaqed

Thanks!
Pulling once the tests settle.

arielshaqed · 2021-02-17T10:19:11Z

api/controller.go

+	return metadata.GetMetaRangeHandlerFunc(func(params metadata.GetMetaRangeParams, user *models.User) middleware.Responder {
+		deps, err := c.setupRequest(user, params.HTTPRequest, []permissions.Permission{
+			{
+				// TODO(oz): May want tighter permissions here.


After f2fing, changed to require both list objects and read repo: it gives a cross between the 2. E.g. with (just) read permission on underlying storage it gives list objects (and without it gives size of branch), which is list-objects-like. But it also gives the storage namespace for the repo, which is read-repo-like. So best to require both.

Returns data (currently just the address of the object) by ID.

It really is a cross between "read repo" (which lets you see namespace on underlying storage) and "list objects" (which you will get, once you read all the files). So require both.

Update The Other swagger.yml.

arielshaqed requested review from nopcoder and ozkatz and removed request for nopcoder and ozkatz February 16, 2021 07:42

arielshaqed force-pushed the feature/add-metadata-location-endpoint branch from e19b271 to 4cf999d Compare February 16, 2021 08:00

nopcoder approved these changes Feb 17, 2021

View reviewed changes

arielshaqed linked an issue Feb 17, 2021 that may be closed by this pull request

Add endpoint to fetch locations of ranges and metaranges on backing store #1472

Closed

arielshaqed commented Feb 17, 2021

View reviewed changes

arielshaqed requested a review from ozkatz February 17, 2021 09:47

arielshaqed commented Feb 17, 2021

View reviewed changes

arielshaqed force-pushed the feature/add-metadata-location-endpoint branch from edd394c to e996175 Compare February 17, 2021 10:59

arielshaqed added 3 commits February 17, 2021 13:14

Add "get metarange", "get range" API endpoints

7527c75

Returns data (currently just the address of the object) by ID.

[CR] Change "{Meta,}RangeData" to "{Meta,}RangeInfo"

3928145

[CR] Add "list objects" permission to read {meta,}range file URLs

f581e49

It really is a cross between "read repo" (which lets you see namespace on underlying storage) and "list objects" (which you will get, once you read all the files). So require both.

arielshaqed force-pushed the feature/add-metadata-location-endpoint branch from e996175 to f581e49 Compare February 17, 2021 11:14

arielshaqed added 2 commits February 17, 2021 13:17

make gen-ui

dcef40c

Update The Other swagger.yml.

[bugfix] restore "default" stanza dropped during merge

d05d64d

arielshaqed merged commit f2ed086 into master Feb 17, 2021

arielshaqed deleted the feature/add-metadata-location-endpoint branch February 17, 2021 12:08

arielshaqed mentioned this pull request Feb 22, 2021

Thick client should receive data URLs from server #1428

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add "get metarange", "get range" API endpoints #1465

Add "get metarange", "get range" API endpoints #1465

arielshaqed commented Feb 16, 2021

codecov-io commented Feb 16, 2021 •

edited

Loading

nopcoder left a comment

nopcoder Feb 17, 2021

arielshaqed Feb 17, 2021

nopcoder Feb 17, 2021

arielshaqed Feb 17, 2021

arielshaqed Feb 17, 2021

nopcoder Feb 17, 2021

arielshaqed left a comment

arielshaqed Feb 17, 2021

arielshaqed Feb 17, 2021

arielshaqed commented Feb 17, 2021

arielshaqed left a comment

arielshaqed Feb 17, 2021

Add "get metarange", "get range" API endpoints #1465

Add "get metarange", "get range" API endpoints #1465

Conversation

arielshaqed commented Feb 16, 2021

codecov-io commented Feb 16, 2021 • edited Loading

Codecov Report

nopcoder left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arielshaqed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arielshaqed commented Feb 17, 2021

arielshaqed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Feb 16, 2021 •

edited

Loading