Upgrade Thrift and Client Libs #33
Conversation
Now that we have cleaned up the namespace for thrift generated PHP files we no longer have collisions on the `Query` object so we can have both of the following: ``` \ThriftSQL\Query \ThriftGenerated\Query ``` This cleans up the namespace a bit for our class.
This applies the diff in 61eb605 to our Thrift generated PHP files at build time. > Rename `echo` method to `echo_str` in Beeswax Service > > This changes the interface of Beeswax exposed to PHP however it's needed > because `echo` is a reserved keyword.
Summary of Changes
|
| $this->_username = self::USERNAME_DEFAULT; | ||
| } | ||
| if ( empty( $this->_password ) ) { | ||
| $this->_password = 'ANY-PASSWORD'; |
There was a problem hiding this comment.
Is it ok to pass a non-null password values. Will this work when authentication is really enabled?
There was a problem hiding this comment.
I'm unsure, there's very little documentation on any of this... I'm basically piecing together what needs to be done with Thrift calls using just whatever is exposed in the interfaces and looking through HiveServer2/Impala server code to figure out what can be done. In theory this change only sets a random password if none is already passed in which fixes an invalid use case of setting a username but no password with SASL in Hive connections. I'm not sure what happens when secured auth is actually turned on. We'll need to test when that happens I guess.
| } | ||
|
|
||
| public function close() { | ||
| try { |
There was a problem hiding this comment.
should we wrap all exceptions in a \ThriftSQL\Exception?
There was a problem hiding this comment.
So I'm just catching and ignoring with a finally only the Impala service documents that it can throw an exception on close but feels like we can safely ignore any potential issues as there's no way to actually recover from not being able to close a query handle. (Because that just means something is totally broken and the query handle is invalid anyway.)
build.patch
Outdated
| @@ -0,0 +1,24 @@ | |||
|
|
|||
There was a problem hiding this comment.
Should we put an explanation somewhere as to the why we need to patch this?
I assume it's coming from Thrift generation? is there a relevant issue somewhere maybe?
There was a problem hiding this comment.
Good point I'll see if I can add some comments to the patch file.
There was a problem hiding this comment.
So instead of commenting on why we need the patch I committed 0e856d2 which switches out the the existing classmap generator so that we no longer need to do this patch.
| insert_final_newline = true | ||
| charset = utf-8 | ||
| indent_style = space | ||
| indent_size = 2 |
There was a problem hiding this comment.
:ewwww:
( it's Friday, I gotta troll 
)
There was a problem hiding this comment.
Yeah I thought about changing the indents here but figured maybe that's a good first bug for someone else at some point. Just matching with what the existing code is doing. :)
Start using `theseer/autoload` instead of `jesseschalken/autoload-generator` to generate classmap used for autoloading the PHAR. This is done because the jesseschalken version is quite old now, last update 2016, and is using `nikic/php-parser` 1.4 which has a bug during tokenization. That bug causes `echo()` method definitions to be confused with `echo()` function redefinitions forcing us to use a monkey patch to change the Thrift Beeswax interface to avoid using `BeeswaxServiceIf::echo()`.
By switching out to the newer `theseer/autoload` classmap generator, last update May 2019, we get to use PHP's "native" `ext/tokenizer` extension instead which does a better job tokenizing and does not have a problem with `echo()` methods allowing us to get rid of the hacky monkey patch and follow the Thrift service defination more closely. The only downside of this new classmap generator does not force preloading of `Thrift/Transport/TSocketPool.php` which the classmap generator did due to the use of `function_exists('apc_fetch')` in that file. In theory this lazyloading should not matter in our particular case but may change if `TSocketPool` starts trying to do something fancier then a noop ployfill of APC.
This repo is getting long in the tooth, let's upgrade Thrift and the associated libs for Hive/Impala and implement users for Impala.