fix(queue): fixes bug in SqlQueue doContainsMessage to handle multiple batches

[ivorp]

A small bug fix which ensures the SqlQueue implementation of containsMessage works when there are more messages than the currently hardcoded batch size (100).

This problem was discovered by the ZombieExecutionService detecting false zombie executions because the underlying SqlQueue.doContainsMessage implementation wasn not searching through all batches of messages available.

[ivorp]

The underlying problem here is a little subtle. The bug appears to be the result of the ResultSet.next() overshooting the cursor at the end of searching a batch.

The resulting ResultSet.row() check below never equals the expected batchSize and so no further batches are queried when they should.

[deverton]

This whole bit of code seems like it could do with some restructuring. It's half-way between native JDBC and JOOQ and makes it hard to reason about because it's mixing concepts.

Given that JOOQ fetches the entire result set in to memory anyway, I don't see a reason to not just use its Result object directly and break out of the search if a matching message is found or the result set is empty.

Maybe something like?

    do {
      val rs: Result<Record3<Any, Any, Any>> = withRetry(READ) {
        jooq.select(idField, fingerprintField, bodyField)
          .from(messagesTable)
          .where(idField.gt(lastId))
          .limit(batchSize)
          .fetch()
      }

      rs.forEach { record ->
        val body = record.getValue("body", String::class.java)
        try {
          if (predicate.invoke(mapper.readValue(body))) return true
        } catch (e: Exception) {
          log.error("Failed reading message with fingerprint: ${record.getValue("fingerprint", String::class.java)} message: $body", e)
        }
        lastId = record.getValue("id", String::class.java)
      }
    } while (rs.isNotEmpty)

    return false

[deverton]

That could be tidied up further by adding some types to the field declarations (idField etc), but that would be a bigger change.

[ivorp]

Didn't want to come empty handed without tests. However I could not find a nice place for SqlQueue specific test implementation.

Open to suggestions on what to do with these - or simply remove them if this fix doesn't warrant the additional testing overhead.

[deverton]

Given that the tests identify an edge case I think they definitely need to be added. Is there a way to add them without changing the visibility of the doContainsMessage() function?