Destination connector implementation. #1
Conversation
destination/destination.go
Outdated
|
|
||
| filename, ok := record.Metadata["filename"] | ||
| if !ok { | ||
| filename = string(record.Key.Bytes()) |
There was a problem hiding this comment.
What if the key contains some characters that would make an invalid file name? E.g. the key could be a structured ket.
There was a problem hiding this comment.
This(filename missing from metadata) should never happen, record should have the filename metadata, but just to handle this we can use hash for filename?
There was a problem hiding this comment.
That's true, the filename should not be missing, if the record came from an SFTP source. But Conduit should be able to combine different sources, and so, for example, it should be possible to sync files from S3 to an SFTP destination.
The key could actually be the file name?
There was a problem hiding this comment.
Okay, so do you mean if record.Key is of type opencdc.StructuredData which is basically a map, then there should be a key "filename" which could be used here?
There was a problem hiding this comment.
I meant that the key can be a string (i.e. opencdc.RawData), similar to what we have in the S3 source.
destination/destination.go
Outdated
| filename = string(record.Key.Bytes()) | ||
| } | ||
|
|
||
| err = d.sftpClient.Rename(path, fmt.Sprintf("%s/%s", d.config.DirectoryPath, filename)) |
There was a problem hiding this comment.
IIUC, this won't keep the original directory structure?
There was a problem hiding this comment.
Not the original one like in source, but it will use the directoryPath in the config.
There was a problem hiding this comment.
IMHO, if we're syncing directories (and we do, because the SFTP source has a dir parameter, and the SFTP destination has a dir parameter too), then it would be expected that the dir. structure is kept.
There was a problem hiding this comment.
Okay,
We have "directoryPath" in both source and destination so that we can read from one dir in source and write to another dir in destination. The connector just syncs the files inside the source directory into destination directory.
So dir structure is kept as per destination dir parameter.
There was a problem hiding this comment.
Oh, then I misunderstood something about the source. I thought that the source directory is traversed recursively, but that doesn't appear to be the case, right? E.g. if I have:
/source/dir1/file1
/source/file2
then only file2 will be read?
There was a problem hiding this comment.
I see. In that case, the destination code makes sense, but we should call this out in the README, that the connector will sync only those files that are in the source dir itself.
| }) | ||
| } | ||
|
|
||
| func TestDestination_Write(t *testing.T) { |
There was a problem hiding this comment.
It would be really good if we had a test for chunking too.
…ctor-sftp into feat/destination
README.md
Outdated
| Destination connects to a remote server. It takes an `opencdc.Record`, extracts filename from the metadata and upload the file to the remote server. The connector supports both password and private key authentication methods. | ||
|
|
There was a problem hiding this comment.
Chunking is a nice feature and it would be good to mention in the readme that we support it and in which way (what metadata is needed, and so on), so that other sources (not only SFTP) can use it.
…ctor-sftp into feat/destination
…ctor-sftp into feat/destination
|
LGTM, thanks @parikshitg! |
Description
Destination implementation for SFTP connector.
Quick checks: