-
Notifications
You must be signed in to change notification settings - Fork 1.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Feature][Connector-V2] [Amazondynamodb Connector]add amazondynamodb source & sink connnector #3166
Conversation
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.
add docs into this ?
https://github.com/apache/incubator-seatunnel/tree/dev/docs/en/connector-v2
<maven.compiler.source>8</maven.compiler.source> | ||
<maven.compiler.target>8</maven.compiler.target> |
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.
remove
Thank you for your codereview,and this pr is still under development. I will fix all the problems. |
PTAL @hailin0 @Hisoka-X @EricJoy2048 @ic4y @CalvinKirs thanks. |
|
||
private final AmazondynamodbSourceOptions amazondynamodbSourceOptions; | ||
private final SeaTunnelRowType seaTunnelRowType; | ||
private final DynamoDbClient dynamoDbClient; |
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.
Does DynamoDbClient support serialization? Under different engines, the construction and write methods of the SinkWriter object may not be called in the same jvm.
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.
SinkWriter will in same jvm
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.
My fault, sinkwriter doesn't need serialization.
@Override | ||
public void write(SeaTunnelRow element) throws IOException { | ||
|
||
dynamoDbClient.putItem(convertRow(element, seaTunnelRowType)); |
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.
Is putItem a synchronous write, can I use batch write to improve performance
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.
Is putItem a synchronous write, can I use batch write to improve performance
Is putItem a synchronous write, can I use batch write to improve performance
Done.
|
||
## Key features | ||
|
||
- [x] [batch](../../concept/connector-v2-features.md) |
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.
Sink Connector Features only contain exactly-once
and schema projection
.
- [ ] [parallelism](../../concept/connector-v2-features.md) | ||
- [ ] [support user-defined split](../../concept/connector-v2-features.md) |
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.
Ditto
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.
Done.
Don't worry, This is a bug about engine, fixed by #3216 |
scheduledFuture.cancel(false); | ||
scheduler.shutdown(); | ||
} | ||
flush(); |
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.
The close method will also be called when no data is written, and dynamoDbClient will have a null pointer exception at this time. This can be tested by a task where the source does not output any data.
public int batchIntervalMs = DEFAULT_BATCH_INTERVAL_MS; | ||
|
||
public AmazondynamodbSourceOptions(Config config) { | ||
if (config.hasPath(AmazondynamodbConfig.URL)) { |
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.
Seem like these config don't have default value, maybe you should use CheckConfigUtil.checkAllExists
to make sure user didn't forget config it.
public int batchIntervalMs = DEFAULT_BATCH_INTERVAL_MS; | ||
|
||
public AmazondynamodbSourceOptions(Config config) { | ||
if (config.hasPath(AmazondynamodbConfig.URL)) { |
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.
You already check it exist, No need to check again
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.
You already check it exist, No need to check again
Done.
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.
LGTM @Hisoka-X @EricJoy2048 PTAL
Purpose of this pull request
Check list
New License Guide