File References Pagination #273
Conversation
shravanshetty1
requested review from
kushthedude
and removed request for
vivekv96 and
shravanshetty1
| UpdatedAt time.Time `gorm:"column:updated_at" dirlist:"updated_at" filelist:"updated_at"` | ||
|
|
||
| DeletedAt gorm.DeletedAt `gorm:"column:deleted_at"` // soft deletion | ||
| ID int64 `gorm:"column:id;primary_key" json:"id,omitempty"` |
There was a problem hiding this comment.
Why add json annotations explicitly? Gorm handles the json naming imo.
There was a problem hiding this comment.
It gives flexibility and also I am not sure about Gorm handling json naming. It supports json dataTypes but it always fills struct fields with its value and its easy to use it with json tag.
There was a problem hiding this comment.
json column field is used when we unmarshal an interface to the following dataType, but since we already have gorm annotation, gorm has default serialising strategy as snake_case hence I said this change is redundant.
There was a problem hiding this comment.
I don't have context on 0fs and the pagination followed here, according to the original issue the need was to have /v1/file/list/{allocation} accept query params & support pagination. Since I am still new to the project, I have just pointed out the general golang improvements.
Can I add it later after completing 0FS? |
|
@lpoli resolve the conflicts, else lgtm |
There was a problem hiding this comment.
Hey @lpoli, CI is complaining will you please check into it, also if you like let's make this change too, #273 (comment) either is fine with me 😅
CI test is error is fixed. |
| @@ -34,6 +34,14 @@ type ReferencePathResult struct { | |||
| LatestWM *writemarker.WriteMarker `json:"latest_write_marker"` | |||
| } | |||
|
|
|||
| type RefResult struct { | |||
| TotalPages int `json:"total_pages"` | |||
| NewOffsetPath string `json:"offsetPath,omitempty"` | |||
There was a problem hiding this comment.
could you add some documentation about what is NewOffsetPath/NewoffsetDate? it is confused there without any comment.
There was a problem hiding this comment.
total_pages is snake_case, and offsetPath is lower_case. should we use snake_case only?
There was a problem hiding this comment.
and the field name is NewOffsetPath, but it is offsetPath in json. why they are different?
| "github.com/0chain/blobber/code/go/0chain.net/core/common" | ||
| ) | ||
|
|
||
| func checkValidDate(s string) error { |
There was a problem hiding this comment.
how about Timezone? it will be an issue if client send date with local time zone.
| func GetUpdatedRefs(ctx context.Context, allocationID, path, offsetPath, _type, updatedDate, offsetDate string, level, pageLimit int) (refs *[]Ref, totalPages int, newOffsetPath, newOffsetDate string, err error) { | ||
| var totalRows int64 | ||
| var pRefs []Ref | ||
| db := datastore.GetStore().GetDB() |
There was a problem hiding this comment.
Yes.db code smells off. but i have not idea yet.
|
requested changes will be fixed on #325 |
ISSUE
This branch solves the above issues by paginating File References which is used to construct ObjectTree, ObjectPath and ReferencePath. I've created new handler that provides paginated fileRefs.
There are three types of fileRefs: regular(undeleted refs), updated(updated refs compared to some given time) and deleted(deleted refs compared to some given time) which is crucial to construct local Tree cache to Add, Update and Delete tree nodes.
Pagination is done using alternative of offset https://blog.jooq.org/2013/10/26/faster-sql-paging-with-jooq-using-the-seek-method/
This makes use of indexes which significantly improves search performance.
Default and maximum pageLimit is 100 rows(about 50KB to 100KB json response). Client can request to obtain single reference with pageLimit 1. GoSdk work is in progress which serves as validating consensus of fileRefs. So that means actual tree construction is upto the application that is build upon gosdk; For example, zboxCli, 0fs, s3fs, ipfs, etc.