Persist and partition job #3

artursouza · 2024-03-08T07:57:02Z

Multi-tenancy with namespace
Partitioning for jobs to be spread across hosts
Persist on Etcd
GitHub workflow with testing
Persist with Protobuf
Handle TTL

Signed-off-by: Artur Souza <asouza.pro@gmail.com>

artursouza · 2024-03-08T07:59:41Z

cron.go

+	// The type of trigger
+	TriggerType string
+	// The payload containg all the information for the trigger
+	TriggerPayload []byte


I will convert this to Any again and use protobuf internally to save the state.

Were you going to do this change in this PR or a follow up? I guess it wont hurt to merge since we aren't referencing the commit in dist scheduler yet, but still feel we should update it first.

artursouza · 2024-03-08T08:00:12Z

cron.go

-	Func func(context.Context) error
-
-	Repeats  int32
-	DueTime  string


I don't want to add this here until we add support for each one of these. Keeping the original Rhythm only for now.

We will want to keep this in mind then bc I believe this exists and is sent to the ScheduleJob so it'll error during our testing once your PR is merged

Adding it here is not enough if the cron scheduler does not handle it.

I'm already passing it along, so if I update the scheduler to use these changes it will break and I'll have to comment out those lines in the scheduler. I mean, its def not production code, but it's still partly there and being referenced and sent along. If you want I can comment them out. Just seems like extra orchestration

artursouza · 2024-03-08T08:10:41Z

cron.go

-	nonAlphaNumerical = regexp.MustCompile("[^a-z0-9_]")
-)
-
-func (j Job) canonicalName() string {


This should not be a limit in the library. It can be in Dapr.

Signed-off-by: Artur Souza <asouza.pro@gmail.com>

artursouza · 2024-03-09T03:42:05Z

I can do it here but want to see if Josh can explain how to do it with Nix before I bring a Makefile.

…

On Fri, Mar 8, 2024 at 7:40 PM Cassie Coyle ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In cron.go <#3 (comment)>: > - -var ( - nonAlphaNumerical = regexp.MustCompile("[^a-z0-9_]") -) - -func (j Job) canonicalName() string { - return strcase.ToSnake( - nonAlphaNumerical.ReplaceAllString( - strings.ToLower(j.Name), - "_", - ), - ) + // The type of trigger + TriggerType string + // The payload containg all the information for the trigger + TriggerPayload []byte Were you going to do this change in this PR or a follow up? I guess it wont hurt to merge since we aren't referencing the commit in dist scheduler yet, but still feel we should update it first. — Reply to this email directly, view it on GitHub <#3 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAA77CWJYOASCXPA3XCLSRLYXKACPAVCNFSM6AAAAABEMMAUPKVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTSMRWGA4DIMBSGM> . You are receiving this because you authored the thread.Message ID: ***@***.***>

cicoyle · 2024-03-09T03:45:23Z

I personally don't know Nix, and for consistency sake would prefer Makefiles. However, if he wants to take that ownership and make it easy enough for us to consume and use that's fine.

organizer.go

artursouza · 2024-03-09T05:08:29Z

I personally don't know Nix, and for consistency sake would prefer Makefiles. However, if he wants to take that ownership and make it easy enough for us to consume and use that's fine.

Let me do it with a simple Makefile and Josh can convert to Nix or help us convert it.

Signed-off-by: Artur Souza <asouza.pro@gmail.com>

store.go

JoshVanL · 2024-03-11T19:26:06Z

store.go

+				}
+			}
+		}
+		log.Printf("Exited sync for path: %s\n", prefix)


Do we need to be logging this? If we need to, please pass a logger into the library and make Debug level.

Not needed. Let me delete it. I want to handle logging as a separate PR.

store.go

proto/job.proto

cron.go

JoshVanL · 2024-03-11T20:07:38Z

@artursouza btw, the tests hang for me on TestNoEntries.

cron.go

artursouza · 2024-03-12T00:35:16Z

@artursouza btw, the tests hang for me on TestNoEntries.

You need to run etcd first.

Signed-off-by: Artur Souza <asouza.pro@gmail.com>

cicoyle · 2024-03-12T13:29:41Z

@artursouza btw, the tests hang for me on TestNoEntries.

You need to run etcd first.

I remember running into this as well. Can you update the README or add a README to the test folder noting this:

## Tests

Pre-requisites to run the tests locally:
- [Install etcd](https://etcd.io/docs/v3.4/install/) 
- Launch etcd: `etcd --logger=zap`
- Proceed to run tests: `go test`

I have this pending in the PR here that I haven't circled back to yet.

cicoyle · 2024-03-12T13:52:20Z

I updated my local wip branch to point to this and the scheduleJob hangs then gives context deadline exceeded:

CASSIE: before scheduling job
2024/03/12 08:46:12 CASSIE: after scheduling job: rpc error: code = DeadlineExceeded desc = context deadline exceeded

Maybe I need to change things before trying the code changes, but that should probably error nicer?

Did you run the tests and update in a branch the go mod and other changes needed in scheduler as a test for these changes?

JoshVanL

After running the test with ETCD I get a data race:

==================
WARNING: DATA RACE
Write at 0x00c0007ce658 by goroutine 206:
  go.etcd.io/etcd/client/v3/concurrency.(*Mutex).tryAcquire()
      /home/josh/go/pkg/mod/go.etcd.io/etcd/client/v3@v3.5.12/concurrency/mutex.go:111 +0x13c
  go.etcd.io/etcd/client/v3/concurrency.(*Mutex).Lock()
      /home/josh/go/pkg/mod/go.etcd.io/etcd/client/v3@v3.5.12/concurrency/mutex.go:72 +0x40
  github.com/diagridio/go-etcd-cron.(*Cron).run.func1()
      /home/josh/go/src/github.com/diagridio/go-etcd-cron/cron.go:431 +0x530
  github.com/diagridio/go-etcd-cron.(*Cron).run.func2()
      /home/josh/go/src/github.com/diagridio/go-etcd-cron/cron.go:447 +0x60

Previous write at 0x00c0007ce658 by goroutine 205:
  go.etcd.io/etcd/client/v3/concurrency.(*Mutex).tryAcquire()
      /home/josh/go/pkg/mod/go.etcd.io/etcd/client/v3@v3.5.12/concurrency/mutex.go:111 +0x13c
  go.etcd.io/etcd/client/v3/concurrency.(*Mutex).Lock()
      /home/josh/go/pkg/mod/go.etcd.io/etcd/client/v3@v3.5.12/concurrency/mutex.go:72 +0x40
  github.com/diagridio/go-etcd-cron.(*Cron).run.func1()
      /home/josh/go/src/github.com/diagridio/go-etcd-cron/cron.go:431 +0x530
  github.com/diagridio/go-etcd-cron.(*Cron).run.func2()
      /home/josh/go/src/github.com/diagridio/go-etcd-cron/cron.go:447 +0x60

Goroutine 206 (running) created at:
  github.com/diagridio/go-etcd-cron.(*Cron).run()
      /home/josh/go/src/github.com/diagridio/go-etcd-cron/cron.go:405 +0xe80
  github.com/diagridio/go-etcd-cron.(*Cron).Start.func1()
      /home/josh/go/src/github.com/diagridio/go-etcd-cron/cron.go:342 +0x4c

Goroutine 205 (running) created at:
  github.com/diagridio/go-etcd-cron.(*Cron).run()
      /home/josh/go/src/github.com/diagridio/go-etcd-cron/cron.go:405 +0xe80
  github.com/diagridio/go-etcd-cron.(*Cron).Start.func1()
      /home/josh/go/src/github.com/diagridio/go-etcd-cron/cron.go:342 +0x4c
==================
==================
WARNING: DATA RACE
Write at 0x00c0007ce668 by goroutine 206:
  go.etcd.io/etcd/client/v3/concurrency.(*Mutex).tryAcquire()
      /home/josh/go/pkg/mod/go.etcd.io/etcd/client/v3@v3.5.12/concurrency/mutex.go:123 +0x73c
  go.etcd.io/etcd/client/v3/concurrency.(*Mutex).Lock()
      /home/josh/go/pkg/mod/go.etcd.io/etcd/client/v3@v3.5.12/concurrency/mutex.go:72 +0x40
  github.com/diagridio/go-etcd-cron.(*Cron).run.func1()
      /home/josh/go/src/github.com/diagridio/go-etcd-cron/cron.go:431 +0x530
  github.com/diagridio/go-etcd-cron.(*Cron).run.func2()
      /home/josh/go/src/github.com/diagridio/go-etcd-cron/cron.go:447 +0x60

Previous write at 0x00c0007ce668 by goroutine 205:
  go.etcd.io/etcd/client/v3/concurrency.(*Mutex).tryAcquire()
      /home/josh/go/pkg/mod/go.etcd.io/etcd/client/v3@v3.5.12/concurrency/mutex.go:123 +0x73c
  go.etcd.io/etcd/client/v3/concurrency.(*Mutex).Lock()
      /home/josh/go/pkg/mod/go.etcd.io/etcd/client/v3@v3.5.12/concurrency/mutex.go:72 +0x40
  github.com/diagridio/go-etcd-cron.(*Cron).run.func1()
      /home/josh/go/src/github.com/diagridio/go-etcd-cron/cron.go:431 +0x530
  github.com/diagridio/go-etcd-cron.(*Cron).run.func2()
      /home/josh/go/src/github.com/diagridio/go-etcd-cron/cron.go:447 +0x60

Goroutine 206 (running) created at:
  github.com/diagridio/go-etcd-cron.(*Cron).run()
      /home/josh/go/src/github.com/diagridio/go-etcd-cron/cron.go:405 +0xe80
  github.com/diagridio/go-etcd-cron.(*Cron).Start.func1()
      /home/josh/go/src/github.com/diagridio/go-etcd-cron/cron.go:342 +0x4c

Goroutine 205 (finished) created at:
  github.com/diagridio/go-etcd-cron.(*Cron).run()
      /home/josh/go/src/github.com/diagridio/go-etcd-cron/cron.go:405 +0xe80
  github.com/diagridio/go-etcd-cron.(*Cron).Start.func1()
      /home/josh/go/src/github.com/diagridio/go-etcd-cron/cron.go:342 +0x4c
==================
==================
WARNING: DATA RACE
Write at 0x00c0007ce670 by goroutine 206:
  go.etcd.io/etcd/client/v3/concurrency.(*Mutex).Lock()
      /home/josh/go/pkg/mod/go.etcd.io/etcd/client/v3@v3.5.12/concurrency/mutex.go:79 +0x16c
  github.com/diagridio/go-etcd-cron.(*Cron).run.func1()
      /home/josh/go/src/github.com/diagridio/go-etcd-cron/cron.go:431 +0x530
  github.com/diagridio/go-etcd-cron.(*Cron).run.func2()
      /home/josh/go/src/github.com/diagridio/go-etcd-cron/cron.go:447 +0x60

Previous write at 0x00c0007ce670 by goroutine 205:
  go.etcd.io/etcd/client/v3/concurrency.(*Mutex).Lock()
      /home/josh/go/pkg/mod/go.etcd.io/etcd/client/v3@v3.5.12/concurrency/mutex.go:79 +0x16c
  github.com/diagridio/go-etcd-cron.(*Cron).run.func1()
      /home/josh/go/src/github.com/diagridio/go-etcd-cron/cron.go:431 +0x530
  github.com/diagridio/go-etcd-cron.(*Cron).run.func2()
      /home/josh/go/src/github.com/diagridio/go-etcd-cron/cron.go:447 +0x60

Goroutine 206 (running) created at:
  github.com/diagridio/go-etcd-cron.(*Cron).run()
      /home/josh/go/src/github.com/diagridio/go-etcd-cron/cron.go:405 +0xe80
  github.com/diagridio/go-etcd-cron.(*Cron).Start.func1()
      /home/josh/go/src/github.com/diagridio/go-etcd-cron/cron.go:342 +0x4c

Goroutine 205 (finished) created at:
  github.com/diagridio/go-etcd-cron.(*Cron).run()
      /home/josh/go/src/github.com/diagridio/go-etcd-cron/cron.go:405 +0xe80
  github.com/diagridio/go-etcd-cron.(*Cron).Start.func1()
      /home/josh/go/src/github.com/diagridio/go-etcd-cron/cron.go:342 +0x4c
==================
    testing.go:1465: race detected during execution of test
--- FAIL: TestDelayedAdd (12.00s)

store.go

partition.go

proto/job.proto

Signed-off-by: Artur Souza <asouza.pro@gmail.com>

artursouza added 3 commits March 7, 2024 23:28

WIP: change lock to be per job.

b74c773

Signed-off-by: Artur Souza <asouza.pro@gmail.com>

Lock per job + namespace handing.

4f98fb7

Signed-off-by: Artur Souza <asouza.pro@gmail.com>

Partitioning of jobs.

39cca47

Signed-off-by: Artur Souza <asouza.pro@gmail.com>

artursouza commented Mar 8, 2024

View reviewed changes

artursouza force-pushed the persist branch from f139811 to db7046d Compare March 8, 2024 08:12

job store.

e3ec85f

Signed-off-by: Artur Souza <asouza.pro@gmail.com>

artursouza force-pushed the persist branch from db7046d to e3ec85f Compare March 8, 2024 08:18

GH workflow to test.

9798f8a

Signed-off-by: Artur Souza <asouza.pro@gmail.com>

artursouza mentioned this pull request Mar 9, 2024

GH workflow to test. #4

Closed

cicoyle reviewed Mar 9, 2024

View reviewed changes

organizer.go Show resolved Hide resolved

artursouza added 4 commits March 8, 2024 21:09

Remove dependabot automerge workflow.

14fd374

Signed-off-by: Artur Souza <asouza.pro@gmail.com>

Record job using protobuf.

d15b22b

Signed-off-by: Artur Souza <asouza.pro@gmail.com>

Handle TTL.

2ea4fd4

Signed-off-by: Artur Souza <asouza.pro@gmail.com>

Some random fixes.

dec4dad

Signed-off-by: Artur Souza <asouza.pro@gmail.com>

artursouza force-pushed the persist branch from 3e3bda4 to dec4dad Compare March 11, 2024 18:00

Fix TestLocalTimezone flakiness.

28fda86

Signed-off-by: Artur Souza <asouza.pro@gmail.com>

artursouza force-pushed the persist branch from a0c6eac to 096207d Compare March 11, 2024 18:15

Fix more flaky tests.

f2b7548

Signed-off-by: Artur Souza <asouza.pro@gmail.com>

artursouza force-pushed the persist branch from 096207d to f2b7548 Compare March 11, 2024 18:18

JoshVanL requested changes Mar 11, 2024

View reviewed changes

cron.go Outdated Show resolved Hide resolved

artursouza added 2 commits March 11, 2024 17:36

Addressed comments.

61c891b

Signed-off-by: Artur Souza <asouza.pro@gmail.com>

Stop via context only.

79c8fd1

Signed-off-by: Artur Souza <asouza.pro@gmail.com>

artursouza added 2 commits March 11, 2024 18:38

Remove update routine.

c0a4624

Signed-off-by: Artur Souza <asouza.pro@gmail.com>

nit: comment

72bb149

Signed-off-by: Artur Souza <asouza.pro@gmail.com>

JoshVanL requested changes Mar 12, 2024

View reviewed changes

store.go Outdated Show resolved Hide resolved

partition.go Show resolved Hide resolved

proto/job.proto Outdated Show resolved Hide resolved

Fix detected race conditions.

5374c6e

Signed-off-by: Artur Souza <asouza.pro@gmail.com>

artursouza force-pushed the persist branch from 3f846a3 to 5374c6e Compare March 12, 2024 20:43

artursouza added 5 commits March 12, 2024 14:58

Update RADME.md and docs.go

a58d534

Signed-off-by: Artur Souza <asouza.pro@gmail.com>

Addressing more comments.

28e055b

Signed-off-by: Artur Souza <asouza.pro@gmail.com>

Reduce locking contention for cron dist mutex reuse.

496f658

Signed-off-by: Artur Souza <asouza.pro@gmail.com>

Does not try to recover from panic.

63d968c

Signed-off-by: Artur Souza <asouza.pro@gmail.com>

Allow test to take longer.

1958992

Signed-off-by: Artur Souza <asouza.pro@gmail.com>

JoshVanL approved these changes Mar 13, 2024

View reviewed changes

artursouza merged commit 04fed7e into diagridio:master Mar 13, 2024
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Persist and partition job #3

Persist and partition job #3

artursouza commented Mar 8, 2024 •

edited

Loading

artursouza Mar 8, 2024

cicoyle Mar 9, 2024

artursouza Mar 8, 2024

cicoyle Mar 8, 2024

artursouza Mar 8, 2024

cicoyle Mar 9, 2024

artursouza Mar 8, 2024

artursouza commented Mar 9, 2024 via email

cicoyle commented Mar 9, 2024

artursouza commented Mar 9, 2024

JoshVanL Mar 11, 2024

artursouza Mar 11, 2024

JoshVanL commented Mar 11, 2024

artursouza commented Mar 12, 2024

cicoyle commented Mar 12, 2024

cicoyle commented Mar 12, 2024

JoshVanL left a comment

Persist and partition job #3

Persist and partition job #3

Conversation

artursouza commented Mar 8, 2024 • edited Loading

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

artursouza commented Mar 9, 2024 via email

cicoyle commented Mar 9, 2024

artursouza commented Mar 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoshVanL commented Mar 11, 2024

artursouza commented Mar 12, 2024

cicoyle commented Mar 12, 2024

cicoyle commented Mar 12, 2024

JoshVanL left a comment

Choose a reason for hiding this comment

artursouza commented Mar 8, 2024 •

edited

Loading