Github user rmetzger commented on the pull request:
https://github.com/apache/flink/pull/666#issuecomment-100902145
Hey,
thank you for working on this & opening a pull request.
I'll address your comments inline:
> put the default value for queueSize attribute in a constant (we could
call it, DEFAULT_QUEUE_SIZE)
> put the default value for waitSec attribute in a constant (we could call
it, DEFAULT_WAIT_SECONDS)
Good idea, please do so. Could you also make these values configurable for
our users? Nobody wants to recompile Flink just for changing an integer ;)
>i see a couple of methods, that seems to work in paris such as:
>open / close: called at the begining of the function and at the very end
of it
>run / cancel: called on each iteration to perform some work or cancel the
current ongoing work
>Is my understanding correct? I am asking since i see that the
TwitterSource is initializing its client connection in the open(...) method but
stops it in the run(...) method. Do those methods works in pairs?
The way we call sources in Flink streaming is a bit broken, thats why we
are reworking it right now. Thats the pull request where we change it:
https://github.com/apache/flink/pull/659
But you are right, open() and close() work in pairs. Connections we are
opening in the open() method should be closed in close().
I would leave that for now as it is, because we are going to change that in
the next days anyways.
> do you think it is a good idea to assign the client to null after
stopping it in the closeConnection method, so it can be garbage collected as
soon as possible? Also, if closeConnection() throws an exception, isRunning
never changes to false.
Yes, you can set it to null.
> we should check for preconditions in the constructors and public set
methods to avoid receiving null auth propeties or similar cases.
> we could have a private constructor that receives all the possible
parameters and use constructor chaining to avoid code duplication even in the
constructors.
These are both very good ideas!
> I have created a util file to place methods when working with properties
files, i put the load method that loads a properties object form a file path,
we should place this in a more common package, i put it close to the file i
have changed but we should consider moving it to a better place.
Maybe we can combine it with this one:
https://github.com/apache/flink/pull/664
> Separate the different connectors into different submodules.
There is already a JIRA filed for this:
https://issues.apache.org/jira/browse/FLINK-1874
But I would suggest to wait for a few days until
https://github.com/apache/flink/pull/659 is merged, otherwise, there are going
to be a lot of merge conflicts.
In general: Feel free to change whatever is necessary to make the code
better (also the missing javadocs). Your suggestions are all very good. I see
that you have a good sense of writing high quality code!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
