Generalize S3 record writes with wrapper to possibly convert to RetriableException #494
Conversation
There was a problem hiding this comment.
@cjolivier01 Thanks a lot for this change, this is a very good refactoring.
I sent some minor nits, please resolve them and then its ready for a final review
kafka-connect-s3/src/main/java/io/confluent/connect/s3/storage/IORecordWriter.java
Outdated
Show resolved
Hide resolved
kafka-connect-s3/src/main/java/io/confluent/connect/s3/format/S3RetriableRecordWriter.java
Outdated
Show resolved
Hide resolved
kafka-connect-s3/src/main/java/io/confluent/connect/s3/format/S3RetriableRecordWriter.java
Outdated
Show resolved
Hide resolved
| private IORecordWriter writer; | ||
|
|
||
| public S3RetriableRecordWriter(IORecordWriter writer) { | ||
| this.writer = writer; |
There was a problem hiding this comment.
for my understanding, don't we have to do null check on constructor arguments?
There was a problem hiding this comment.
is that a pattern? I looked around just now and I don't see it being done anywhere.
There was a problem hiding this comment.
I am not sure if its the pattern or not, but we could make writer as final and do a null check before assigning a value in constructor. what you think?
There was a problem hiding this comment.
it is final now. do what if it is null? throw NullPointerException?
There was a problem hiding this comment.
I would do something like this: writer = Preconditions.checkNotNull(writer, "Writer object is null"))
we don't have to use guava-preconditions. but checkNotNull will throw NullPointerException.
Ref: https://guava.dev/releases/19.0/api/docs/com/google/common/base/Preconditions.html
There was a problem hiding this comment.
added throw if null
|
I assume we dont have to add any new unit tests for this change, right? |
We don't need to as this part of code has enough coverage added in main PR and this is just a refactoring. |
Problem
Duplicated catch/throw logic for each format's RecordWriter with respect to retriable AWS exceptions.
Solution
Consolidte the catch/rethrow logic for AWS retriable exceptions by wrapping the anonymous format classes as they are created. Wrapper is intentionally as close as possible to the call/throw sites rather than up another level (or more) in KeyValueHeaderRecordWriterProvider.
No behavioral changes should occur.
Suggest turning off whitespace in comparison since blocks of code were simply shifted right one tab in many places.
Does this solution apply anywhere else?
If yes, where?
Test Strategy
Testing done:
Release Plan