adding basic gocron-gorm-lock functionality. #1
Conversation
|
This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation. |
|
@manuelarte thanks for your desire to contribute to the open source! See my comments on the design below. Great work! |
gormlock.go
Outdated
| // This is a complicated thing, we are assuming that the minute precision is enough | ||
| ji := time.Now().Truncate(time.Millisecond).Format("2006-01-02 15:04:05.000") |
There was a problem hiding this comment.
Why only minute precision? Gocron supports seconds, miliseconds, etc. So locker should be able to handle jobs running sub-minute intervals
There was a problem hiding this comment.
it's miliseconds precission, right? .Truncate(time.Millisecond) (I need to fix the stupid comment... sorry about that).
But apart from that, this is a tricky one, and maybe it's worth discussing with an example. In my case I am using the Postgress driver for gorm, in this case Postgres timestamp does not have nanoseconds precission (go-gorm/gorm#3232), so that's why I decided to put it in miliseconds, but I guess this is "driver" dependent. Maybe it's a good idea to allow the user to set it's precission? (and if not, use miliseconds?)
There was a problem hiding this comment.
Yes I think making it configurable. Defaulting to milliseconds seems reasonable though.
What happens if you pass in nanosecond precision to Postgress, will it truncate?
There was a problem hiding this comment.
I made it configurable. Now you can pass how you can create the job identifier. I also added a test and updated the readme.
If you pass nanoseconds to postgres then you get weird behaviours (I think I got the information from here: lib/pq#227)
| func TestEnableDistributedLocking_DifferentJob(t *testing.T) { | ||
| ctx := context.Background() | ||
| postgresContainer, err := testcontainerspostgres.RunContainer(ctx, | ||
| testcontainers.WithWaitStrategy(wait.ForLog("database system is ready to accept connections"). |
There was a problem hiding this comment.
I can't reproduce this alias issue, @JohnRoesler is it possible that you tell me how you get it? do you run a command? or you get it in your IDE?
There was a problem hiding this comment.
The golangci lint step reports the error https://github.com/go-co-op/gocron-gorm-lock/actions/runs/6825095998/job/18568477606
Let me see if I can see what the issue is locally
There was a problem hiding this comment.
Ah, it's because you dont' have a go.sum file. Run go mod tidy and it should solve the issue
There was a problem hiding this comment.
I ran go mod tidy, and I also ran the command that is failing in the pipeline (golangci-lint run --out-format=github-actions), and I still don't get the error. I am sure I am missing something...
There was a problem hiding this comment.
Shall I commit the go.sum file that I get after running go mod tidy?
There was a problem hiding this comment.
yes. Go expects a mod and sum file to be present
|
👍 |
|
I updated the PR |
using suggestion Co-authored-by: John Roesler <[email protected]>
|
Looks like the test is failing |
yes. This is a problem I discovered some time ago. The miliseconds precision (the one I put by default), gives problems since the operation to lock and unlock sometimes takes quite some time. I decided to change the precision to seconds, and change the tests accordingly. |
|
I forgot to update one part of one test |
|
@JohnRoesler could you run the workflows again? |
|
is something else missing? |
What does this do?
Which issue(s) does this PR fix/relate to?
Adding basic gocron-gorm lock functionality. It should be described in the Readme file.
Important topic to discuss:
List any changes that modify/break current functionality
Have you included tests for your changes?
I added a test following the convention in gocron-redis-lock, I also added an extra test that checks the uniqueness of the job name + timestamp in the database.
Did you document any new/modified functionality?
example_test.goREADME.mdNotes
Any feedback is appreciated.