Making Samza-Sql-Shell commands pluggable

[shenodaguirguis]

Adding CommandHandler andCommandType interfaces to make samza-sql-shell commands pluggable through conf

[weiqingy]

Issues we have left untouched but maybe as a future work:

The environment variables (operated by the SET command). Shall a commandhandler have its own environment variables? Shall their environment variables start with shell.? At the moment we treat all environment variables starting with shell. as the Shell's, and pass everything else to the executor. The logic may no longer work. We may need to distinguish them, like shell. , executor., commandhandler1., commandhandler2., etc.

The SET command and multiple commandhandlers. Just like we allow user to change the executor via the SET command, we allow user to change commandhandlers via the SET command. However, with multiple commandhandlers allowed, it's a bit tricky. Say a user has loaded commandhandler1 and commandhandler2. He or she now wants to use commandhandler3 as well. We wouldn't want them to issue a command like:
SET commandhandler = commandhandler1, commandhandler2, commandhandler3
What's in my mind is new commands load and unload. User may issue commands like:

load commandhandler1
load commandhandler2

We may also want to consider distinguish commands of different commandhandlers by adding a prefix on the command, like:

commandhandler1.commandA
commandhandler2.commandA

Let me know what you think.

[shenodaguirguis]

my 2-mites:

• the load/unload commands makes perfect sense, can be a future work
• I am not a big fan of prefixing commands with handlers, not very user friendly? ... besides, with load/unload this won't be needed, but I don't anticipate users shuffling command handlers much, this feature is mainly to allow each user to plug-in their favorite handler.
• We can discuss offline the env vars scope, I don't see them applicable per handler...

[weiqingy]

Yeah. We'll see what we need to do according to the user feedback.

[atoomula]

my 2-mites:

• the load/unload commands makes perfect sense, can be a future work
• I am not a big fan of prefixing commands with handlers, not very user friendly? ... besides, with load/unload this won't be needed, but I don't anticipate users shuffling command handlers much, this feature is mainly to allow each user to plug-in their favorite handler.
• We can discuss offline the env vars scope, I don't see them applicable per handler...

I think we need to differentiate between what environment variables users can load/set dynamically and what can be loaded at shell startup time. Command handler is one which sits in a bucket where it can be loaded only at shell startup time. It makes code also much simpler. I'm thinking about prefixing/name-spacing such variables so that it is easier to identify such env vars. It need not go as part of this PR but what do you guys think ?

[atoomula]

LGTM. We can discuss my other comment offline.

[weiqingy]

@atoomula

I agree.
IMO command handers should NOT be environment variables. They are EXTENSIONs. They shall NOT be loaded by SET command. I prefer a load command instead.

[weiqingy]

@shenodaguirguis Forgot to remind of the Java document on interfaces and public methods. There'll be lots of build warnings otherwise.