deprecate Tink worker global timeout cli flag: #605
Conversation
Codecov Report
@@ Coverage Diff @@
## main #605 +/- ##
==========================================
+ Coverage 45.96% 46.22% +0.25%
==========================================
Files 56 56
Lines 3283 3310 +27
==========================================
+ Hits 1509 1530 +21
- Misses 1684 1690 +6
Partials 90 90
Continue to review full report at Codecov.
|
cmd/tink-worker/cmd/root.go
Outdated
| var cancel context.CancelFunc | ||
| ctx, cancel = context.WithTimeout(ctx, timeOut) | ||
| defer cancel() | ||
| logger.Info("timeOut is deprecated") |
There was a problem hiding this comment.
lets just get rid of the code, we have enough workarounds/deprecated things and it just turns into baggage. We're still at v <1.0 and also have sandbox to provide a cushion for breakages.
jacobweinstock
force-pushed
the
tink-worker-timeout
branch
from
e935a37 to
bc632a3
Compare
cmd/tink-worker/cmd/root.go
Outdated
| ctx := context.Background() | ||
| err = w.ProcessWorkflowActions(ctx) |
There was a problem hiding this comment.
Can you use the command context here?
| ctx := context.Background() | |
| err = w.ProcessWorkflowActions(ctx) | |
| err = w.ProcessWorkflowActions(cmd.Context()) |
There was a problem hiding this comment.
This is currently effectively the same thing. Will change if we do ExecuteContext in cmd/tink-worker/main.go which I think would give us ctrl-c support? For a later PR though.
The Tink worker global timeout cli flag doesn't implement the global timeout defined in a Tink server template correctly. A single template can have multiple workers. So a single Tink worker defining a global timeout for all task is erroneous. It doesn't take the multiple workers idea into account. A long term solution will need to be implemented. Something that takes into account multiple workers. Signed-off-by: Jacob Weinstock <jakobweinstock@gmail.com>
jacobweinstock
force-pushed
the
tink-worker-timeout
branch
from
bc632a3 to
9382cd8
Compare
Description
The Tink worker global timeout cli flag doesn't implement the global timeout defined in a Tink server template correctly.
A single template can have multiple workers. So a single Tink worker defining a global timeout for all tasks is erroneous. It doesn't take the multiple workers idea into account.
A long term solution will need to be implemented. Something that takes into account multiple workers. This PR just removes this erroneous behavior.
Why is this needed
Fixes: #
How Has This Been Tested?
How are existing users impacted? What migration steps/scripts do we need?
Checklist:
I have: