feature: define the interface and http api of preheat #1242

lowzj · 2020-03-12T10:51:43Z

Signed-off-by: lowzj [email protected]

Ⅰ. Describe what this PR did

This pull request defines the interface and http api of preheat.

Ⅱ. Does this pull request fix one issue?

#1115

Ⅲ. Why don't you add test cases (unit test/integration test)? (你真的觉得不需要加测试吗？)

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

codecov-io · 2020-03-12T11:18:10Z

Codecov Report

Merging #1242 into master will decrease coverage by 0.26%.
The diff coverage is 23.75%.

@@            Coverage Diff             @@
##           master    #1242      +/-   ##
==========================================
- Coverage   51.41%   51.14%   -0.27%     
==========================================
  Files         125      126       +1     
  Lines        8241     8321      +80     
==========================================
+ Hits         4237     4256      +19     
- Misses       3661     3720      +59     
- Partials      343      345       +2

Impacted Files	Coverage Δ
supernode/server/preheat_bridge.go	`21.33% <21.33%> (ø)`
supernode/server/server.go	`43.07% <50.00%> (+0.45%)`	⬆️
supernode/server/router.go	`66.10% <100.00%> (+0.58%)`	⬆️

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 0d3e9b2...435eb38. Read the comment docs.

allencloud · 2020-03-18T07:32:30Z

supernode/server/preheat_bridge.go

+
+	"github.com/dragonflyoss/Dragonfly/apis/types"
+	"github.com/dragonflyoss/Dragonfly/pkg/errortypes"
+	"github.com/go-openapi/strfmt"


Please add a blank line here.

allencloud · 2020-03-18T07:34:46Z

supernode/server/preheat_bridge.go

+	id := mux.Vars(req)["id"]
+	task, err := s.PreheatMgr.Get(ctx, id)
+	if err != nil {
+		return err


You defined a code of StatusNotFound, while I am wondering if the code here could return the code of 404?

PreheatMgr returns the error with a proper code and methodhandlePreheatErrorResponse convert the error to error response and send to user.

allencloud · 2020-03-18T07:37:11Z

supernode/daemon/mgr/preheat_mgr.go

+	Get(ctx context.Context, id string) (task *PreheatTask, err error)
+
+	// Delete deletes a preheat task by id.
+	Delete(ctx context.Context, id string) (err error)


Do we need to add the Delete endpoint if the swagger.yml? Since I have not found that.
I am afraid that we should define the API in swagger.yml first, and then implement that in code.

allencloud · 2020-03-18T07:39:11Z

supernode/server/preheat_bridge.go

+			// Handle error if request handling fails.
+			handlePreheatErrorResponse(w, err)
+		}
+		logrus.Debugf("%v err:%v", req.URL, err)


I think we should not print this line if err is equal to nil. But the code here would print that no matter what value the err is.

It's a debug level log that indicates a request has been processed and what result is. It represents the process is success if the err is nil.

lowzj · 2020-03-25T02:47:44Z

@allencloud PTAL

starnop · 2020-03-30T11:37:47Z

supernode/daemon/mgr/preheat_mgr.go

+	return (string)(ps)
+}
+
+// PreheatTask store the detailed preheat task information.


s/store/stores

starnop · 2020-03-30T11:38:44Z

supernode/daemon/mgr/preheat_mgr.go

+)
+
+// PreheatStatus
+type PreheatStatus string


How about defining it with go-swager?

lowzj · 2020-04-08T07:14:35Z

@starnop @allencloud PTAL

starnop · 2020-04-15T03:13:19Z

@starnop @allencloud PTAL

Sorry for the late reply. I will finish the review today.

starnop · 2020-04-15T14:33:54Z

supernode/daemon/mgr/preheat_mgr.go

+	Delete(ctx context.Context, id string) (err error)
+
+	// GetAll gets all preheat tasks that unexpired.
+	GetAll(ctx context.Context) (task []*PreheatTask, err error)


update "task" to "preheatTask" in order to avoid ambiguity.

starnop · 2020-04-15T14:37:17Z

supernode/server/preheat_bridge.go

+	}
+	resp := types.PreheatInfo{
+		ID:         task.ID,
+		FinishTime: strfmt.DateTime{},


use strfmt .NewDateTime() instead?

starnop · 2020-04-15T14:41:59Z

supernode/daemon/mgr/preheat_mgr.go

+	Identifier string
+	Headers    map[string]string
+
+	ParentID   string


What are the fields "ParentID" and "Children" for? And how about add comments here?

lowzj · 2020-04-22T15:56:18Z

@allencloud @starnop PTAL

Signed-off-by: lowzj <[email protected]>

allencloud · 2020-05-08T02:49:03Z

LGTM

Signed-off-by: sunwp <[email protected]> Co-authored-by: Jim Ma <[email protected]>

pouchrobot added kind/feature size/XL labels Mar 12, 2020

lowzj force-pushed the preheat branch from e97798f to 7c5a03d Compare March 12, 2020 11:15

lowzj force-pushed the preheat branch 2 times, most recently from e5b326b to ced0538 Compare March 18, 2020 07:04

allencloud reviewed Mar 18, 2020

View reviewed changes

allencloud suggested changes Mar 18, 2020

View reviewed changes

lowzj force-pushed the preheat branch 2 times, most recently from 4a37394 to 20a39ed Compare March 18, 2020 12:07

lowzj changed the title ~~[WIP] feature: define the interface and http api of preheat~~ feature: define the interface and http api of preheat Mar 18, 2020

pouchrobot added the conflict/needs-rebase label Mar 24, 2020

lowzj force-pushed the preheat branch from 20a39ed to 4e53b91 Compare March 24, 2020 09:01

pouchrobot removed the conflict/needs-rebase label Mar 24, 2020

lowzj force-pushed the preheat branch 2 times, most recently from da7adf1 to 7a31b77 Compare March 24, 2020 14:41

starnop reviewed Mar 30, 2020

View reviewed changes

lowzj force-pushed the preheat branch 2 times, most recently from 157bb4b to a4a4e07 Compare April 8, 2020 07:12

lowzj force-pushed the preheat branch 3 times, most recently from 80304d1 to 4fbbaa1 Compare April 10, 2020 01:18

starnop reviewed Apr 15, 2020

View reviewed changes

lowzj force-pushed the preheat branch 3 times, most recently from 64e65f2 to 99918a7 Compare April 22, 2020 15:49

weekly-digest bot mentioned this pull request Apr 26, 2020

Weekly Digest (19 April, 2020 - 26 April, 2020) #1303

Closed

lowzj force-pushed the preheat branch 5 times, most recently from c6989e8 to 9d34dfd Compare May 1, 2020 09:00

pouchrobot added size/XL and removed size/XL labels May 1, 2020

weekly-digest bot mentioned this pull request May 3, 2020

Weekly Digest (26 April, 2020 - 3 May, 2020) #1323

Closed

feature: define the interface and http api of preheat

435eb38

Signed-off-by: lowzj <[email protected]>

lowzj force-pushed the preheat branch from 9d34dfd to 435eb38 Compare May 7, 2020 23:46

allencloud approved these changes May 8, 2020

View reviewed changes

pouchrobot added the LGTM one maintainer or community participant agrees to merge the pull reuqest. label May 8, 2020

allencloud merged commit 0ca3333 into dragonflyoss:master May 8, 2020

pouchrobot mentioned this pull request May 8, 2020

WeeklyReport of Dragonfly from 2020-05-01 to 2020-05-08 #1328

Closed

sungjunyoung pushed a commit to sungjunyoung/Dragonfly that referenced this pull request May 8, 2022

fix too_many_pings & grpc waitOnHeader block (dragonflyoss#1242)

06e8bc7

Signed-off-by: sunwp <[email protected]> Co-authored-by: Jim Ma <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature: define the interface and http api of preheat #1242

feature: define the interface and http api of preheat #1242

lowzj commented Mar 12, 2020 •

edited

Loading

codecov-io commented Mar 12, 2020 •

edited

Loading

allencloud Mar 18, 2020

lowzj Mar 18, 2020

allencloud Mar 18, 2020

lowzj Mar 18, 2020

allencloud Mar 18, 2020

lowzj Mar 18, 2020

allencloud Mar 18, 2020

lowzj Mar 18, 2020

lowzj commented Mar 25, 2020

starnop Mar 30, 2020

starnop Mar 30, 2020

lowzj commented Apr 8, 2020

starnop commented Apr 15, 2020

starnop Apr 15, 2020

starnop Apr 15, 2020

starnop Apr 15, 2020

lowzj commented Apr 22, 2020

allencloud commented May 8, 2020

feature: define the interface and http api of preheat #1242

feature: define the interface and http api of preheat #1242

Conversation

lowzj commented Mar 12, 2020 • edited Loading

Ⅰ. Describe what this PR did

Ⅱ. Does this pull request fix one issue?

Ⅲ. Why don't you add test cases (unit test/integration test)? (你真的觉得不需要加测试吗？)

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

codecov-io commented Mar 12, 2020 • edited Loading

Codecov Report

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

Choose a reason for hiding this comment

lowzj commented Mar 25, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lowzj commented Apr 8, 2020

starnop commented Apr 15, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lowzj commented Apr 22, 2020

allencloud commented May 8, 2020

lowzj commented Mar 12, 2020 •

edited

Loading

codecov-io commented Mar 12, 2020 •

edited

Loading