Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

distributed cron via temporal #288

Merged
merged 68 commits into from
May 21, 2024
Merged

distributed cron via temporal #288

merged 68 commits into from
May 21, 2024

Conversation

ig-ra
Copy link
Contributor

@ig-ra ig-ra commented May 12, 2024

No description provided.

@ig-ra
Copy link
Contributor Author

ig-ra commented May 12, 2024 •
edited

all framework changes (proto/scheme/sdktypes) are already merged and this PR was rebased on current main.

@ig-ra ig-ra requested review from itayd, efiShtain and daabr May 12, 2024 22:01
@ig-ra ig-ra force-pushed the igor/eng-301-distributed-cron-via-temporal branch 2 times, most recently from 572a06f to c45cbb6 Compare May 13, 2024 09:51
daabr
daabr reviewed May 14, 2024
View reviewed changes
Copy link
Contributor

@daabr daabr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't finish the review, but here are a few comments in the meantime

cmd/ak/cmd/triggers/create.go Outdated Show resolved Hide resolved
manifest.schema.yaml Show resolved Hide resolved
proto/autokitteh/triggers/v1/trigger.proto Outdated Show resolved Hide resolved
internal/backend/db/dbgorm/triggers.go Outdated Show resolved Hide resolved
internal/backend/triggers/triggers.go Outdated Show resolved Hide resolved
@ig-ra ig-ra force-pushed the igor/eng-301-distributed-cron-via-temporal branch from 44ed9e2 to 13313e0 Compare May 15, 2024 13:33
itayd
itayd reviewed May 16, 2024
View reviewed changes
sdk/sdktypes/trigger.go Outdated Show resolved Hide resolved
@ig-ra ig-ra force-pushed the igor/eng-301-distributed-cron-via-temporal branch from 051993b to 544fd22 Compare May 16, 2024 14:10
@ig-ra ig-ra force-pushed the igor/eng-301-distributed-cron-via-temporal branch 3 times, most recently from 2babb05 to f26793c Compare May 20, 2024 06:05
itayd
itayd reviewed May 20, 2024
View reviewed changes
sdk/sdktypes/trigger.go Outdated Show resolved Hide resolved
itayd
itayd reviewed May 20, 2024
View reviewed changes
sdk/sdktypes/trigger.go Outdated Show resolved Hide resolved
itayd
itayd reviewed May 20, 2024
View reviewed changes
sdk/sdktypes/trigger.go Outdated Show resolved Hide resolved
@ig-ra ig-ra force-pushed the igor/eng-301-distributed-cron-via-temporal branch from a9d3c0b to df4a745 Compare May 20, 2024 15:41
itayd
itayd reviewed May 20, 2024
View reviewed changes
sdk/sdktypes/connection.go Outdated Show resolved Hide resolved
itayd
itayd reviewed May 20, 2024
View reviewed changes
internal/manifest/plan.go Outdated Show resolved Hide resolved
itayd
itayd reviewed May 20, 2024
View reviewed changes
internal/manifest/plan.go Outdated Show resolved Hide resolved
@ig-ra ig-ra force-pushed the igor/eng-301-distributed-cron-via-temporal branch 2 times, most recently from 21968d1 to 11bc10e Compare May 20, 2024 23:57
itayd
itayd reviewed May 21, 2024
View reviewed changes
sdk/sdktypes/connection.go
@@ -28,10 +28,16 @@ func (ConnectionTraits) Validate(m *ConnectionPB) error {
}

func (ConnectionTraits) StrictValidate(m *ConnectionPB) error {
var errProjectID, errIntegrationID error
if m.Name != SchedulerConnectionName {
errProjectID = mandatory("project_id", m.ProjectId)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think project id is always needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why? in DB we have projectID is optional. moreover cron connection isn't for a project but global one

@ig-ra ig-ra force-pushed the igor/eng-301-distributed-cron-via-temporal branch from e4d45dc to a5dcaca Compare May 21, 2024 17:41
@ig-ra ig-ra merged commit d6a9a70 into main May 21, 2024
13 checks passed
@ig-ra ig-ra deleted the igor/eng-301-distributed-cron-via-temporal branch May 21, 2024 18:23
daabr
daabr reviewed May 21, 2024
View reviewed changes
cmd/ak/cmd/triggers/create.go

var createCmd = common.StandardCommand(&cobra.Command{
Use: "create <--name=...> <--env=...> <--connection=...> <--event=...> <--loc=...>",
Use: `create <--name=...> <--env=...> <--loc=...> { <--connection=...> <--event=...> | <--schedule=...> }`,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a tiny issue, but you shouldn't use `` quotation marks unless there's a specific reasons for it (regex patterns, strings containing "", or multiline strings).

Also, conceptually, the loc parameter should come last (after defining the trigger conditions, you define what the trigger will do).

cmd/ak/cmd/triggers/create.go
}
event = sdktypes.SchedulerEventTriggerType
data[sdktypes.ScheduleExpression] = sdktypes.NewStringValue(schedule)
connection = sdktypes.SchedulerConnectionName // FIXME: fix resolver. resolving by name isn't working
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this new? Is there a ticket with details?

cmd/ak/cmd/triggers/create.go
data := make(map[string]sdktypes.Value)
if cmd.Flags().Changed("schedule") {
if connection != "" || event != "" {
return fmt.Errorf(`flags(s) "connection", "event" are not compatible with "schedule"`)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As much as possible, you shouldn't write custom logic for this, you should use Cobra's flag modifiers. See MarkFlagsMutuallyExclusive and MarkFlagsRequiredTogether.

daabr added a commit that referenced this pull request May 22, 2024
@daabr
Revert "distributed cron via temporal (#288)"
745ab4d 
This reverts commit d6a9a70.
@daabr daabr mentioned this pull request May 22, 2024
Merged
daabr added a commit that referenced this pull request May 22, 2024
@daabr
Revert "distributed cron via temporal" (#320)
039de6a 
I ran a test for another PR in progress (#319): deploy `samples/github`,
fire event (new issue comment).

I got this error consistently (omitting Temporal from stack trace):

```
2024-05-21 21:25:40 ERROR temporalclient zap@v0.5.0/logger.go:72 Failed processing event {"Namespace": "default", "WorkerID": "51732@abraham.local@", "WorkflowType": "eventsWorkflow", "WorkflowID": "evt_01hyf8rdzdeasr37rftfh8pnfy", "EventID": "evt_01hyf8rdzdeasr37rftfh8pnfy", "error": "expected DispatchOptions isn't provided", "TaskQueue": "events-task-queue", "RunID": "6d571816-4511-4cb7-997f-005b6817a42a", "Attempt": 1}
...
go.autokitteh.dev/autokitteh/internal/backend/dispatcher.(*dispatcher).eventsWorkflow
	/Users/daniel/git/autokitteh/internal/backend/dispatcher/eventsworkflow.go:295
reflect.Value.call
	/usr/local/go/src/reflect/value.go:596
reflect.Value.Call
	/usr/local/go/src/reflect/value.go:380
...
```

After testing builds of previous merges, the first that worked was
pre-#288, so I'm rolling back instead of fixing-forward.

Will sync with @ig-ra to understand the root-cause.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@daabr daabr daabr left review comments

@itayd itayd itayd approved these changes

@efiShtain efiShtain Awaiting requested review from efiShtain

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@ig-ra @itayd @daabr