-
Notifications
You must be signed in to change notification settings - Fork 0
BOR-2992: Set destination stream buffer size to 5mb #3
base: master
Are you sure you want to change the base?
Conversation
NOTE
|
@void666 @jhecking This cannot be configured via environment variables, because env variables are never passed down to the connectors, they're handled at the airbyte-worker level. Plus this exists in the connector base interface, so I don't think it's a good idea to use the |
As discussed, we don't need to use env variables. We can pass the desired max. buffer size as part of the connector config. The new config value also has to be added to the spec for the destination-s3 connector. |
01e1bb1
to
383d9a3
Compare
public FileBuffer(final String fileExtension, final Integer perStreamBufferSizeMb) { | ||
this.fileExtension = fileExtension; | ||
this.maxConcurrentStreams = DEFAULT_MAX_CONCURRENT_STREAM_IN_BUFFER; | ||
this.maxPerStreamBufferSizeBytes = perStreamBufferSizeMb * 1024 * 1024; | ||
tempFile = null; | ||
outputStream = null; | ||
} | ||
|
||
public FileBuffer(final String fileExtension, final int maxConcurrentStreams) { | ||
this.fileExtension = fileExtension; | ||
this.maxConcurrentStreams = maxConcurrentStreams; | ||
this.maxPerStreamBufferSizeBytes = MAX_PER_STREAM_BUFFER_SIZE_BYTES; | ||
tempFile = null; | ||
outputStream = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I am understanding how the constructor overriding will work here
Both are of the same type, both are accepting two parameters.
FileBuffer(xlsx, 100) -> intending to override the maxConcurrentStreams
FileBuffer(xlsx, 200) -> intending to override the perStreamBufferSizeMb
I would recommend using a three-parameter constructor, instead of overriding the two-parameter one. Since the types are castable (Integer -> int, and int -> Integer), the other usages will have an impact if we go with two parameter overrides.
@@ -8,10 +8,13 @@ | |||
|
|||
public interface S3FormatConfig { | |||
|
|||
Integer DEFAULT_FILE_CHUNK_SIZE_MB = 200; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be a derived value from FileBuffer's MAX_PER_STREAM_BUFFER_SIZE_BYTES
. If the default value is changed there, should be reflected here as well.
No description provided.