Re: TikaIO concerns

[Jean-Baptiste Onofré]

Hi Eugene,

fully agree ! My point was more in term of features: I think it's fair to 
postpone some features in an IO in new PRs.
For example, when we created JmsIO, it only supports TextMessage and new message 
types support will be added in new improvement PRs.

It's what I meant by "basically good",  it's more in feature scope.

Regards
JB

On 09/20/2017 01:18 AM, Eugene Kirpichov wrote:
On Tue, Sep 19, 2017 at 5:13 AM Jean-Baptiste Onofré <j...@nanthrax.net>
wrote:

Hi Sergey,

as discussed together during the review, I fully understand the choices
you did.

Your plan sounds reasonable. Thanks !

Generally speaking, in order to give visibility and encourage
contribution, I
think it would make sense to accept a PR if it's basically right (even if
it's
not yet perfect) and doesn't break the build.

This is a wider discussion than the current thread, but I don't think I
agree with this approach.

We have followed a much stricter standard in the past, and thanks to that,
Beam currently has (in my opinion) an extremely high-quality library of
IOs, and Beam can take pride in not being one of "those" open-source
projects that advertise everything but guarantee nothing and are
frustrating to work with, because everything is slightly broken in some way
or another.

I can recall at most 1 or 2 cases where a contributor gave up on a PR due
to the amount of issues pointed out during review - and in those cases, the
PR was usually in a state where Beam would not have benefitted from merging
the issue-ridden code anyway. Basically, a thorough review in all cases
I've seen so far has been a good idea in retrospect.

There may be trivial fixups best done by a committer rather than author
(e.g. javadoc typos), but I think nontrivial, high-level issues should be
reviewed rigorously.

People trust Beam (especially Beam IOs) with their data, and at least the
correctness-critical stuff *must* be done right. Beam also generally
promises a stable API, so API mistakes are forever, and can not be fixed
iteratively [this can be addressed by marking in-progress work as
@Experimental] - so APIs must be done right as well. On the other hand,
performance, documentation, lack of support for certain features etc. can
be fixed iteratively - I agree that we shouldn't push too hard on that
during review.

There's also the mentorship aspect: I think it is valuable for new Beam
contributors to get thorough review, especially for their first
contributions, as a kick-start to learning the best practices - they are
going to need them repeatedly in their future contributions. Merging code
without sufficient review gives them the immediate gratification of "having
contributed", but denies the mentorship. Moreover, most contributions are
made by a relatively small number of prolific "serial contributors" (you
being a prime example!) who are responsive to feedback and eager to learn,
so the immediate gratification I think is not very important.

I think the best way to handle code reviews for Beam is to give it our best
as reviewers, especially for first-time contributors; and if it feels like
the amount of required changes is too large for the contributor to handle,
then work with them to prioritize the changes, or start small and decompose
the contribution into more manageable pieces, but each merged piece must be
high-quality.


I would be happy to help on TikaIO as I did during the first review round
;)

Regards
JB

On 09/19/2017 12:41 PM, Sergey Beryozkin wrote:
Hi All

This is my first post the the dev list, I work for Talend, I'm a Beam
novice,
Apache Tika fan, and thought it would be really great to try and link
both
projects together, which led me to opening [1] where I typed some early
thoughts, followed by PR [2].

I noticed yesterday I had the robust :-) (but useful and helpful) newer
review
comments from Eugene pending, so I'd like to summarize a bit why I did
TikaIO
(reader) the way I did, and then decide, based on the feedback from the
experts,
what to do next.

Apache Tika Parsers report the text content in chunks, via SaxParser
events.
It's not possible with Tika to take a file and read it bit by bit at the
'initiative' of the Beam Reader, line by line, the only way is to handle
the
SAXParser callbacks which report the data chunks. Some parsers may
report the
complete lines, some individual words, with some being able report the
data only
after the completely parse the document.
All depends on the data format.

At the moment TikaIO's TikaReader does not use the Beam threads to parse
the
files, Beam threads will only collect the data from the internal queue
where the
internal TikaReader's thread will put the data into
(note the data chunks are ordered even though the tests might suggest
otherwise).

The reason I did it was because I thought

1) it would make the individual data chunks available faster to the
pipeline -
the parser will continue working via the binary/video etc file while the
data
will already start flowing - I agree there should be some tests data
available
confirming it - but I'm positive at the moment this approach might yield
some
performance gains with the large sets. If the file is large, if it has
the
embedded attachments/videos to deal with, then it may be more effective
not to
get the Beam thread deal with it...

2) As I commented at the end of [2], having an option to concatenate the
data
chunks first before making them available to the pipeline is useful, and
I guess
doing the same in ParDo would introduce some synchronization issues
(though not
exactly sure yet)

One of valid concerns there is that the reader is polling the internal
queue so,
in theory at least, and perhaps in some rare cases too, we may have a
case where
the max polling time has been reached, the parser is still busy, and
TikaIO
fails to report all the file data. I think that it can be solved by
either 2a)
configuring the max polling time to a very large number which will never
be
reached for a practical case, or 2b) simply use a blocking queue without
the
time limits - in the worst case, if TikaParser spins and fails to report
the end
of the document, then, Bean can heal itself if the pipeline blocks.
I propose to follow 2b).


Please let me know what you think.
My plan so far is:
1) start addressing most of Eugene's comments which would require some
minor
TikaIO updates
2) work on removing the TikaSource internal code dealing with File
patterns
which I copied from TextIO at the next stage
3) If needed - mark TikaIO Experimental to give Tika and Beam users some
time to
try it with some real complex files and also decide if TikaIO can
continue
implemented as a BoundedSource/Reader or not

Eugene, all, will it work if I start with 1) ?

Thanks, Sergey

[1] https://issues.apache.org/jira/browse/BEAM-2328
[2] https://github.com/apache/beam/pull/3378

--
Jean-Baptiste Onofré
jbono...@apache.org
http://blog.nanthrax.net
Talend - http://www.talend.com



--
Jean-Baptiste Onofré
jbono...@apache.org
http://blog.nanthrax.net
Talend - http://www.talend.com