sql/importer: TestImportWorkerFailure failed about cockroach HOT 7 OPEN

failed, checking that imported data was completely removed: "pq: job 952221520862117889: node liveness error: restarting in background"

error executing 'SELECT * FROM t ORDER BY i': pq: relation "t" is offline: importing

Sounds like the logic here is not robust enough for retry-able failures:

from cockroach.

It looks like a node died during this test, but I see no clues as to why.

from cockroach.

The test itself stops one node in the cluster. In this case it seems that the import was paused due to the node failure, so the table remained offline - this seems like expected behavior in some cases.

from cockroach.

The test itself stops one node in the cluster.

That's unrelated to the error, though, isn't it? Otherwise we'd hit this case every time.

from cockroach.

I think it is related. IIUC what happens is the following:

• we have a distributed import plan. When the import starts, the table is taken offline
• after some time we shut down one of the nodes
• import has an internal retry loop, so it can usually survive that single node failure and actually finish. After finishing, the table is taken online
• however, depending on at which point in time the node death is recognized, that retry loop might not retry the error and return it to the client. In this case we currently check that all imported data was rolled back (as of 14ef3c0).

I think what's missing is that in the last bullet point the table might be still offline, and this is what this test failure tells us. I'm not sure whether it is always offline or only sometimes offline in case the import fails.

from cockroach.

IIUC the table is still offline because the import is being retried: restarting in background. So this is an internal retry error that we don't properly trap?

from cockroach.

Hm, this error message is used in two places where the job coordinator detects a context cancellation error. The error message suggests that the node that is coordinating the import job is overloaded. The import's retry loop exits if it detects the context cancellation, so it's expected that we won't succeed here.

Note that this is a race config on the engflow environment. We enabled race config somewhat recently in #117906 - perhaps we'll need to skip this test under race now that we've transitioned to running race CI on the engflow.

from cockroach.