Philip Zeyliger has posted comments on this change. (
http://gerrit.cloudera.org:8080/11874 )
Change subject: IMPALA-7738: Implement timeouts for HDFS open calls
......................................................................
Patch Set 4:
> (1 comment)
>
> > Thanks!
> >
> > I was a little surprised to see hdfs-monitored-ops have different
> > signatures for the cached vs. non-cached file handles.
> Ultimately,
> > we're trying to implement hdfsOpenWithTimeout(), and I would have
> > expected existing code to be amended to use that. Equivalently,
> > could we be using lambdas or similar to have a totally generic
> > "timeout-pool" for this sort of operation?
>
> I'm flexible about the interface in terms of file handle vs hdfs
> open, etc. The hdfsOpenWithTimeout() might be a better interface.
I think what I find weird is that hdfs-monitored-ops knows the details of
handle-cache.h. It seems like it should be strictly "below" that.
In terms of error, I'm not sure I follow. It seems like:
TimeoutRunError s = pool.runWithTimeout(timeout, []() { hdfsOpen(inputs,
&output); })
could work. TimeoutRunError needs to distinguish between success, failure
without having run the command, and failure with having run the command. The
last one means that we have to poison things that we gave it, since we have no
idea if they'll ever get reclaimed. (Maybe that implies we need to give it a
cleanup operation too?)
Anyway--I don't necessarily think we need to be super generic. And maybe my
Java roots are showing :)
> What other non-HDFS
> operations were you planning on adding timeouts for? Would they
> have their own pools? Would they share a pool with HDFS?
Talking to Kudu is probably in the same boat, but, yeah, we don't have many of
these.
--
To view, visit http://gerrit.cloudera.org:8080/11874
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia14403ca5f3f19c6d5f61b9ab2306b0ad3267454
Gerrit-Change-Number: 11874
Gerrit-PatchSet: 4
Gerrit-Owner: Joe McDonnell <[email protected]>
Gerrit-Reviewer: Impala Public Jenkins <[email protected]>
Gerrit-Reviewer: Joe McDonnell <[email protected]>
Gerrit-Reviewer: Philip Zeyliger <[email protected]>
Gerrit-Reviewer: Tim Armstrong <[email protected]>
Gerrit-Comment-Date: Thu, 08 Nov 2018 02:44:30 +0000
Gerrit-HasComments: No
