Github user carloscurotto commented on the pull request:
https://github.com/apache/flink/pull/666#issuecomment-100671079
Let me also suggest a few other further improvements:
TwitterSource:
- 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)
- 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?
- 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.
- 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.
PropertiesUtil:
- 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.
General:
- In general i saw a few clases that are missing javadoc and validation of
input parameters.
- Separate the different connectors into different submodules.
---
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.
---
