Conversation
|
To play devil's advocate, by that logic, one could just have one single configuration for everything in streams and an enum that could delegate the parameters where necessary. I simply, and made back-wards compatible, a configuration that is tailored to each end-point that one could pull from Twitter. This allows for clarity of usage by the caller and ensures erroneous parameters aren't possible to be constructed which would be implicitly ignored by the the operation. IE: Tracking a stream for keyword, would be different than tracking individuals by ID. This is reflected in Twitter's API documentation. To mirror these parameters ensures the caller can use the module without having to have explicit knowledge of the inner workings of the provider. I hope that this isn't too far reaching, but code clarity seems of upmost importance to ensure proper adoption by a wider community. |
There was a problem hiding this comment.
Did you mean to do something here?
There was a problem hiding this comment.
Good catch, another commit coming with a RUN-TIME exception.
There was a problem hiding this comment.
While this should work fine, you might want to consider just sending the current persist queue and constructing a new instance. That way, you don't have to incur the overhead of copying and clearing the queue.
Not critical
There was a problem hiding this comment.
There is a utility I found the other day that wraps this in a convenience method:
org.apache.streams.util.ComponentUtils.offerUntilSuccess
There was a problem hiding this comment.
Seems like StatusCounterMonitorRunnable is a misnomer for this interface, given the operations I see here. Maybe there is another intent I can't infer?
There was a problem hiding this comment.
There is heavy code duplication in the LocalStreamBuilders that should re-factored to an interface, I'm not picky on the name, but, seeing as the behaviors are consistent and consistently applied, it is JBPs to use an interface to more accurately describe the behavior.
There was a problem hiding this comment.
I think that it does, right now, the monitor thread doesn't shutdown properly because of the way that it is being tracked. This behavior is largely invisible because it would have to be run in debug mode for it to be immediately apparent. IMHO, anything that can be run as a thread against in LocalStreamsBuilder that has a while(this.isRunning) { } loop, should have an interface that allows them to be terminated generically. Otherwise, it is possible to miss the destruction of it. With that style of loop, even through references are terminated, it will continue to run unless the flag is set and it can escape the function.
There was a problem hiding this comment.
Completely agree on the use of the interface. I was just wondering why it is StatusCounterMonitorRunnable when its methods are shutdown and isRunning. It would make sense to me to call it StoppableRunnable or something like that; but, I may be misunderstanding the use case and the Java Doc on the interface doesn't clarify it for me.
|
Overall, lots of great improvements. Thanks for the submission. Given the size of the patch, you need to have an ICLA on file in order for us to contribute it. I see here http://people.apache.org/committer-index.html that your name is listed in the Unlisted CLAs, so I think we are good so long as that is you and not another Matthew Hager. |
|
That is me, that is not another Matthew Hager. Smashew == Matthew Hager |
|
Went back through, ready every comment. I think I have satisfied them all, thank you for the feedback and mentorship through this process, you all are so swell! :) |
4 participants
Some modifications to Twitter streams