optimize: reduce the waiting time for starting dfget server #1088
Conversation
lowzj
force-pushed
the
optimize-server-start
branch
from
baa7538 to
8573a82
Compare
dfget/core/downloader/downloader.go
Outdated
| @@ -42,7 +42,8 @@ type Downloader interface { | |||
| // the given timeout duration. | |||
| func DoDownloadTimeout(downloader Downloader, timeout time.Duration) error { | |||
| if timeout <= 0 { | |||
| logrus.Warnf("invalid download timeout(%.3fs)", timeout.Seconds()) | |||
| logrus.Debugf("invalid download timeout(%.3fs), use default:(%.3fs)", | |||
There was a problem hiding this comment.
I think it is better to use Warnf, since it is a useful message for operators.
There was a problem hiding this comment.
Yes, it's better to use WARN level.
But it makes user confused because that this invalid value is set by dfget not specified by user. I will open another pr to fix it.
| func waitForStartup(result chan error, p2pPtr *unsafe.Pointer) (err error) { | ||
| ticker := time.NewTicker(5 * time.Millisecond) | ||
| defer ticker.Stop() | ||
| timeout := time.After(233 * time.Millisecond) |
There was a problem hiding this comment.
why to use this value 233 * time.Millisecond. How about declaring it a const value?
There was a problem hiding this comment.
It's not a multiple of 5, is a prime number. And it's enough for retry to start a http server. 😄
There was a problem hiding this comment.
In fact, it's weird for 233ms. Maybe other contributors will feel the same way. Could you please add a comment for it? In addition, how about declaring it a const value as @allencloud said?
There was a problem hiding this comment.
why is 233 ms, not others?
It's weird for us because of unfamiliarity, but it's really the one. There are many things in the world that are unexpected. I have already commented on this function to describe why this number jumps out by itself. It jumps into my brain, and I have to write it down.
why not declare it as a const value
I think that maybe it's unnecessary to declare it as a const value. The code here can explain its meaning by a very simple and clear way by itself: it's the timeout of waiting for starting server. Just like the variable ticker with 5ms above. It will lose its meaning when it appears in any other places. And the important thing is that it's very hard to come up with a name of the constant value which can explain itself without context.
lowzj
force-pushed
the
optimize-server-start
branch
from
8573a82 to
633bf7c
Compare
Codecov Report
@@ Coverage Diff @@
## master #1088 +/- ##
==========================================
+ Coverage 47.28% 47.32% +0.04%
==========================================
Files 118 118
Lines 7225 7235 +10
==========================================
+ Hits 3416 3424 +8
- Misses 3540 3541 +1
- Partials 269 270 +1
Continue to review full report at Codecov.
|
Signed-off-by: lowzj <[email protected]>
lowzj
force-pushed
the
optimize-server-start
branch
from
633bf7c to
d03b207
Compare
|
LGTM. |
optimize: reduce the waiting time for starting dfget server
optimize: reduce the waiting time for starting dfget server
Signed-off-by: Gaius <[email protected]>
Signed-off-by: lowzj [email protected]
Ⅰ. 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