[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [HTCondor-users] Python condor_chirp client available for testing




> On Feb 6, 2018, at 4:34 PM, Jason Patton <jpatton@xxxxxxxxxxx> wrote:
> 
> Brian,
> 
> Thanks for review! I'll try to address all of these before we get to a
> "v1.0", but to follow up on a few...
> 
> On Mon, Feb 5, 2018 at 3:30 PM, Brian Bockelman <bbockelm@xxxxxxxxxxx> wrote:
>> 1.  This seems to have copied the condor_chirp behavior of having no
>> timeouts and/or hanging indefinitely on remote error.  Probably worth
>> changing... might I suggest 20 second timeouts?
> 
> The socket object should be created with a default timeout of 10
> seconds. Did I miss setting that somewhere?

Ah - great, I had overlooked that.

It might someday be useful to switch from a socket timeout to an operation timeout (so, if it takes 5 seconds to send a command, you only block for up to 5 seconds instead of a full 10) -- but this should suffice.

> 
>> 2.  There's actually a few chirp commands that can be done without setting
>> IO proxy -- sending delayed updates comes to mind.
> 
> Cool! I thought that was necessary for $_CONDOR_CHIRP_CONFIG to exist,
> but indeed that's not the case. I'll have to think about how to handle
> this case.
> 
> (Test on init? Leave up to user? The proxy returns INVALID_REQUEST if
> using an unsupported command.)
> 

Not sure if it's easily testable to see what the starter supports (after all, future version of HTCondor may behave subtly differently).  I'd suggest leaving it to the user and having nice error messages / codes.

After all, there are some opaque limits in the default mode - such a limits on the number of attributes you can set.

>> 3.  Can we add documentation to http://htcondor-python.readthedocs.io?
> 
> This would be ideal! Definitely can't leave the docs in their current
> state of shuffling users back and forth between help() and the
> condor_chirp manual.

:) Would be happy to accept PRs!

> 
>> 4.  You probably want the object to behave as a context manager to avoid
>> tons of connects / disconnects:
>> 
>> import htchirp
>> with htchip.HTChirp() as chirp:
>>    chirp.ulog("I'm using HTChirp!")
>>    chirpt.set_job_attr("UsingHTChirp", "True")
>> 
>> It'd be straightforward to avoid this disconnects, and it'll also make the
>> code exception safe.  For example, it's possible to have a dangling
>> connection here:
>> 
>> https://github.com/htcondor/htchirp/blob/master/htchirp/htchirp.py#L654
> 
> Yes, in retrospect, this is how I should have done it. And will do
> it... I'll make this change a priority.

Luckily, it's not too big of a change!

> 
>> 5.  Do we need to worry about sending the server responses greater than 1024
>> bytes?  I seem to recall that the starter code in that area seemed a bit ...
>> crashy ... with large responses.  Even if it doesn't cause a crash, it might
>> be worthwhile to have a reasonably good error message if too-long filenames
>> can cause protocol violations.
>> 6.  What Unicode encoding issues should we think about?  That's always been
>> a bit mysterious to me in HTCondor land -- and potentially problematic as
>> Python 3 marches forward.
> 
> Definitely would be good to know, I have neglected at this point to
> shovel much "garbage" at the chirp proxy to see what happens.

Honestly, I usually lean on Jaime for advice when it comes to this -- he recalls the quoting rules for ClassAds, therefore, obviously, he's in charge of Unicode for all of HTCondor.  :)

> 
>> 7.  Would love to see some appveyor / Travis-CI style integration tests!
> 
> I will need some guidance in setting this up, but for sure this would be useful.
> 

Appveyor I can provide very little guidance on.  I'm not sure I've had a Windows license for a decade...

Travis-CI: probably the best place to start is the integration in HTCondor itself!

Brian