Indication that an insert was skipped because of a uniqueness constraint

[chrboe]

Hi, first of all, I love this project, nice work!

I have a setup where I am periodically running checks that might trigger River jobs. So, every 15 minutes, there is the possibility that a bunch of jobs will be started in a batch.

However, these are long-running jobs, and they frequently take longer than the check interval.
So, after running for 15 minutes, the checker logic would discover the same situation as on the previous run and trigger a job that is already running.

The solution to that is easy enough: I use a uniqueness constraint to ensure that only one of these jobs (with the same params) can be run at a time.

Now, the only issue I have – as silly as it sounds – is with logging. With each checker run, I log an overview of all the jobs that have been triggered and for what reason. The problem is that I cannot distinguish between a "successful" job insert and one that was ignored due to the uniqueness constraint. This makes my log outputs confusing because it is a mix of already-running jobs and jobs that were actually started on this run.

Is there a way to know that a job was actually inserted?

I actually just noticed the UniqueSkippedAsDuplicate field on JobInsertResult; this seems to be exactly what I am looking for!
Unfortunately, that value does not seem to get propagated through to the "external" API.

Is it somehow possible to work around this, or to include this in the external API?

[bgentry]

I’m wondering if there’s any good way to do this aside from returning a special error when the duplicate job isn’t inserted. My fear is this would be a bit unwieldy to users needing to disambiguate this error from others that could happen when inserting 🤔

From what I’m reading right now it seems like the docs are definitely not clear what happens in this situation, we should probably explain that more clearly as a starting point.

[chrboe]

Personally, I would have expected one of two results:

1. Returning a special error, as you describe. This feels like the most idiomatic solution to me, but I agree that this would at least need to be clearly documented to cause any confusion.
2. A RowsAffected field on the returned value. That would either be 0 or 1, depending on how many jobs were actually inserted.

I guess the UniqueSkippedAsDuplicate field serves the same purpose as 2., and is maybe a little bit cleaner.

[brandur]

I'm not sure I love the idea of returning an error in this case because although it's a little exceptional maybe, having a job skipped as duplicate doesn't necessarily mean that anything untowards happened. Most users probably won't care about about the insert being skipped (as that's the purpose of the unique system), but will be checking the result with the normal if err != nil { ... }, meaning a lot of people would see their code error unexpectedly.

I think what we maybe should've done is having the insert functions return something like an InsertResult that contains a JobRow for each one. Most users probably don't need to access job properties right after they just inserted the job, so it wouldn't too much more syntax in most cases, and having an InsertResult would give us a place to put other insert-related metadata, like whether the insert was skipped due to uniqueness.

The ship's probably sailed on that level of API change already though, so I wonder if what we could do is invert that design? So something like:

type JobRow struct {
     // all the normal fields
    ...

    InsertResult *InsertResult
}

type InsertResult struct {
    UniqueSkippedAsDuplicate bool
}

InsertResult would be nil for job rows that were not just inserted.

The advantage of this design compared to putting UniqueSkippedAsDuplicate right on JobRow is that it'd let us encapsulated any other "metadata"-like properties that we might want to add in the future.

Alternatively, I could also see adding a property like UniqueSkippedAsDuplicate as a function on JobRow like:

func (r *JobRow) UniqueSkippedAsDuplicate() bool {
    ...
}

Being a function compared to a normal property would flag that it's "special" and not like the other job properties that are more inherent to the job record.

@bgentry Thoughts?

[bgentry]

Of those options I probably lean more toward including an optional field on the JobRow. Although it's a little awkward, I think I even prefer it to if we were able to change the API to return a wrapper struct instead of a JobRow because most use cases will probably care more about the other fields on the JobRow than they'll care about the optional insert properties.

[tadejsv]

What about just creating a public-facing JobInsertResult (return value for InsertTx), structured like this

type JobInsertResult struct {
  UniqueSkippedAsDuplicate bool
  Job JobRow
}

this should make the new property "out of band" (brought up in #142), and would make adding new such properties easier in the future

[brandur]

Hi Tadej,

Yes, this would work, but there's a compatibility tradeoff that we have to factor in. We can't change the return type of Insert/InsertTx because that'd be a backward incompatible change.

We could add a new set of functions like InsertWithResult/InsertWithResultTx that return the job inside a result, but we'd have to weigh that against putting something on JobRow like I pitched above. A new embed on JobRow is a little ugly, but a new set of functions is a little ugly too because it expands the API and makes it a little harder to understand.

[tadejsv]

Yeah, I totally get this dilemma - I proposed this change since you're still in 0.0.X, so I assume some breaking changes would be permissible

[brandur]

Ah yeah, technically we could do it, but based on issue/PR/discussion activity here, I think we have enough users that we're passed the point where a change like this would be annoying, so we're broadly trying to keep all changes backwards compatible.

[chrboe]

Hate to be a bother here, but are there any plans for how to implement this before the API turns "really stable"? Just getting a bit nervous here because the 0.1.0 release seems to move this project in the direction of a more stable API (which is otherwise a great thing, of course 🙂)

I liked @bgentry's idea with the wrapper struct above; I think it would nicely address the concerns noted in the approach I suggested in #142. Are there any "blockers" left that would prevent this from being implemented?

[bgentry]

@chrboe no worries, I imagine this is something we will sort out before we get to v1. While we are still hoping to avoid breaking changes, we're not totally opposed to them where they make sense.

I'm sure you've seen that we've been pretty active on this project, just not on this specific issue. Hope we can get to it soon. ✌️

[chrboe]

Yep, I'm following it all with excitement 🙂 Thanks for your efforts 💪

[brandur]

Opened #292.