cleanup sqlline*.py using python idioms.

[btbytes]

[joshelser]

Hi @btbytes! Thanks for the changes so far. A couple of things:

1. Have you opened a JIRA issue which captures the changes you'd like to make? We want to make sure that JIRA is the source system of record for discussing changes. You should be able to create an "Improvement" issue under https://issues.apache.org/jira/browse/PHOENIX. If you haven't already, please do so, and then let me know the URL and I can assign it to you to for credit.
2. Any thoughts about making the corresponding changes to sqlline.py that you made here to sqlline-thin.py?
3. Any thoughts about testing these changes?

[btbytes]

1. Issue - https://issues.apache.org/jira/browse/PHOENIX-3132
2. I was waiting for a response do this PR before I took on more refactoring ;)
3. I have tested in so far as the output of the two versions of the program are the same before they are shelled out in the last line. So, yes.

[joshelser]

Great! I've given you karma and assigned PHOENIX-3132 to you.

I was waiting for a response do this PR before I took on more refactoring ;)

Hah! Smart :)

I have tested in so far as the output of the two versions of the program are the same before they are shelled out in the last line. So, yes.

Let me clarify. I was wondering if, in your changes to be a bit more idiomatic, you had made any changes which would help us work towards automated unit tests which we could run during the normal Phoenix Maven build.

I've had it on my backburner to look at what Apache Ambari does to invoke unit tests on python code and try to borrow some of that code so that we can try to improve the quality of our python scripts and increase our confidence in future changes (such as your's).

I don't want to push that onto you, but was just curious if you had written any sort of tests as a part of these changes.

[joshelser]

Left a few comments for you inline. Overall, I like the changes. Would love to see corresponding ones made for sqlline.py initially (we tend to make changes to one in the other as well). I don't think we would need to revisit all of the scripts for the first round (unless you're up to the task!).

[btbytes]

I'd definitely love add unit tests by breaking up the functionality into testable parts. I have couple of concerns:

1. the use of globals, esp in the common module -- phoenix_utils. If we manage to isolate modules cleanly, testing will that much more meaningful
2. quite a bit of this script is to poke the environment .. which is I/O. Unit testing may not yield much. But, i'll take a fresh look.

[joshelser]

quite a bit of this script is to poke the environment .. which is I/O. Unit testing may not yield much. But, i'll take a fresh look.

Yeah, I can understand where you're coming from. It would probably require some indirection to do it well. One concrete example I saw again was that we use JAVA_HOME from the environment unless it exists in the hbase-env.* script. These are the sorts of things that stand out at me that, long term, we should have tests to prove work as expected.

Again, I don't want to push them all on you in this changeset, but I'd be more than happy to work with someone who is a better python dev than myself :)

[btbytes]

1. I have added what I think is cross-platform way of handling zombie? child processes. However, I realise there are more than one such call in each program. I'll have atleast one more push to address this.
2. Except for a few lines, both the programs are quite similar. There is an opportunity to reuse code. But this is not an immediate concern.

[joshelser]

I have added what I think is cross-platform way of handling zombie? child processes.

*nux is definitely the one we care about the most :). I would expect that subprocess would already have the platform-specific stuff abstracted away for us (but maybe not)?

However, I realise there are more than one such call in each program. I'll have atleast one more push to address this.

Cool.

Except for a few lines, both the programs are quite similar. There is an opportunity to reuse code. But this is not an immediate concern.

Agreed. Definitely an area for continued improvements!

[joshelser]

Sorry for the delay @btbytes, I missed your latest push where you addressed sqlline.py too. Thanks!

I think this is looking good. I left one comment for you. I will try to test this out locally today.

[btbytes]

Please test this locally. I don't have a working setup ATM.

[joshelser]

Alright, I'll try to poke at this one today.

[joshelser]

% ./bin/sqlline.py
Traceback (most recent call last):
  File "./bin/sqlline.py", line 139, in <module>
    main()
  File "./bin/sqlline.py", line 132, in main
    preexec_fn=os.setsid)
  File "/usr/local/Cellar/python/2.7.12/Frameworks/Python.framework/Versions/2.7/lib/python2.7/subprocess.py", line 711, in __init__
    errread, errwrite)
  File "/usr/local/Cellar/python/2.7.12/Frameworks/Python.framework/Versions/2.7/lib/python2.7/subprocess.py", line 1343, in _execute_child
    raise child_exception
OSError: [Errno 63] File name too long

Looks like there is an upper limit on the command that can be executed. Can you turn the heredoc-style string into an array?

[btbytes]

I'm looking into this. I tried splitting the triple quoted string on space but that isn't working.

[joshelser]

re-ping @btbytes :). Still looking at finishing this up? It would be a very nice improvement for 4.9.0.

[btbytes]

wow. it's been a month. Sorry. I'll get back to this over this weekend.

[joshelser]

No worries! Just cleaning up my inbox and thought about it again. Thank you for returning to it :)