-
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
[api-draft][connector] Add simplified connector api #2041
Conversation
bbbb11f
to
1e5602e
Compare
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.
+1
CC @Hisoka-X @CalvinKirs
Hi, please resolve conflict |
1e5602e
to
955e369
Compare
|
||
public class FakeSourceSplit implements SourceSplit { | ||
public class SingleSplitReaderContext { |
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.
Why doesn't org.apache.seatunnel.connectors.seatunnel.common.source.SingleSplitReaderContext
extend SourceReader.Context
rather than composition?
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.
Using composition prevents developers from using the SourceReader.Context#sendSplitRequest
and SourceReader.Context#sendSourceEventToCoordinator
methods. If we use implements
, we need to throw exceptions in this methods.
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.
It's a little unintelligible that SingleSplitReaderContext
is NOT a subclass of SourceReader.Context
, How about following alternatives:
SingleSplitReaderContext
implementsSourceReader.Context
, and removesendSplitRequest
sendSourceEventToCoordinator
fromSourceReader.Context
if possible. I think these method may not belong Context Object.- Use implements, and throw exceptions. That also makes sense.
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 instantiation of SourceReader.Context
is hidden, so although SingleSplitReaderContext can implements SourceReader.Context
, it must be combined with the internal SourceReader.Context
.
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.
Sorry, I still don't understand why ParallelReaderContext
can implement SourceReader.Context
but SingleSplitReaderContext
can not?
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.
ParallelReaderContext
belongs to the translation module, and the translation module can define how to translate the api to the source of the engine (that is, how the reader and the enumerator interact is determined by the engine, etc.);
SingleSplitReaderContext
belongs to the connector module, and it cannot define how the Context
actually works.
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.
ParallelReaderContext
belongs to the translation module, and the translation module can define how to translate the api to the source of the engine (that is, how the reader and the enumerator interact is determined by the engine, etc.);
SingleSplitReaderContext
belongs to the connector module, and it cannot define how theContext
actually works.
Thx for your explanation. I can better understand translation module
@@ -24,12 +24,7 @@ | |||
*/ | |||
public interface SeaTunnelRuntimeEnvironment { |
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.
xxxAware
is a better class name when there is only onesetXXX
method.
Refer to spring framewok
org.springframework.web.context.ServletConfigAware ``org.springframework.context.ApplicationContextAware
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's good idea! Can you contribute to it?
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.
No problem
Purpose of this pull request
Add simplified connector api
Check list
New License Guide