test(acceptance): add acceptance tests #6
Conversation
donatorsky
force-pushed
the
test/acceptance-tests
branch
4 times, most recently
from
a39670f
to
b8cbb4a
Compare
cmd/blob/acceptance_test.go
Outdated
| }, | ||
|
|
||
| Skip: []string{ | ||
| // When restarting snapshot position, bucket is read from the beginning |
There was a problem hiding this comment.
IIRC, only the object metadata is read from the beginning, but the object contents, are not?
There was a problem hiding this comment.
Object's contents is also read while iterating over storage.
There was a problem hiding this comment.
And from what we know so far, it's not possible to avoid that? It could be come a bottleneck.
There was a problem hiding this comment.
Object's contents is downloaded separately:
https://github.com/miquido/conduit-connector-azure-storage/blob/main/source/iterator/snapshot.go#L118-L123
So you can potentially adjust the logic 🙂
There was a problem hiding this comment.
Thanks, that's a useful reference!
| // Cannot be tested. driver.WriteToSource() creates all records at once while CDC position is | ||
| // created after the Snapshot iterator finishes iterating. So from the CDC iterator point of view, | ||
| // there are no new records available. | ||
| "TestAcceptance/TestSource_Open_ResumeAtPositionCDC", |
There was a problem hiding this comment.
Can you clarify this a little bit further please? Also, what's the actual error you're getting?
TestSource_Open_ResumeAtPositionCDC is doing the following:
- opens a source, which has no records in it
- reads all the records
- acknowledges half of those
- restarts the source
Regardless of the internals of a connector, this use case is something which should pass.
There was a problem hiding this comment.
@lovromazgon also pointed out that these 4 lines should give the source enough time to switch from doing a snapshot (without data) and switch to CDC. The timeout was lower in one of the previous versions of the SDK, so that might have been an issue for you. What he suggested is to upgrade to the latest SDK.
There was a problem hiding this comment.
It is not the matter of time required for connector to switch from the Snapshot iterator to CDC. To make this test working, you would need to change the TestSource_Open_ResumeAtPositionCDC test logic:
want creates all files in the source and snapshot iterator sees them all. CDC iterator does not see any changes.
There was a problem hiding this comment.
want creates all files in the source and snapshot iterator sees them all. CDC iterator does not see any changes.
I think here we have the answer. The expectation in this test is that there's an empty snapshot, after which the connector should switch to CDC. i.e. we have:
- open source, which has no data in it
- start reading -> no data, so no snapshot, and we start listening for changes.
There was a problem hiding this comment.
The important thing to note in that test is that it first tries to read something from the source and expects to get back a context.DeadlineExceeded or sdk.ErrBackoffRetry (see these lines). This means that when the connector first tries to do a snapshot it should discover that there is no data, meaning that the initial snapshot is empty and it should switch to CDC. Any new record that is created after that is considered a change of the existing state and not part of the existing state (existing state is empty).
We need a read timeout because we can't wait forever. The Read method is expected to block until it has a record to return, and there should be no records, so it would block forever. You can adjust the timeout if 5 seconds (default) is not enough to determine that there are no initial records to put into the snapshot.
If you don't think that is correct, I'd be interested to hear what you think should be the criteria for switching from "snapshot" to "CDC"?
There was a problem hiding this comment.
The check for ErrSnapshotIteratorIsStopped is already present in Read method:
https://github.com/miquido/conduit-connector-azure-storage/blob/1dc53a90881cd32092fbd3b92ea34e7b1146eba2/source/source.go#L98-L102
Then converted to ErrBackoffRetry and retried. Ideally would be to update the S3 and this connector logic so that it changes to CDC iterator earlier. I don't think it is in the scope of this PR, though.
There was a problem hiding this comment.
So you are saying the scope of this PR is to run acceptance tests and ignore the tests that fail? I would expect the scope to include fixing the issues found by the acceptance tests (unless they are unreasonably big issues). But let's not argue about technicalities, if you want to fix it in a follow-up PR that's fine by me.
There was a problem hiding this comment.
Is the test failing because there really is an issue, or is the test itself too general and does not support such a real use case? You will face the same problem for each source that does not support consistent iteration (true cursors/scrolls, instead of "just iterate over the current list of items and ignore the fact it may change over the time of iteration"). You are already facing this problem for S3 and Azure connectors.
This fact is already documented: https://github.com/miquido/conduit-connector-azure-storage/blob/main/README.md?plain=1#L20. IMO, "fixing" this is actually implementing a way of determining which file has already been iterated and skipping it. It definitely falls into "unreasonably big issue" bag, and it deserves a separate work+PR.
There was a problem hiding this comment.
I'm not sure I'm able to understand the connection between this tests failure and the fact that Azure nor S3 support true cursors/scrolls/CDC?
If we take a step back, this test is about using CDC after we are done with the snapshot (be it empty or not). The Azure Storage connector has a CDC mode, so we expect the connector to switch to that mode.
There was a problem hiding this comment.
We discussed this at a standup. It looks like the best path forward is to accept the acceptance tests as they are, and work on improving the snapshot/CDC behavior separately. The issue is here.
Thanks @lovromazgon @donatorsky for sharing your thoughts!
donatorsky
force-pushed
the
test/acceptance-tests
branch
2 times, most recently
from
05f7c49
to
7ccc2ce
Compare
donatorsky
force-pushed
the
test/acceptance-tests
branch
from
7ccc2ce
to
c93ab87
Compare
Description
Add acceptance tests as described here:
ConduitIO/conduit-connector-sdk#14
Quick checks: