Skip to content

Conversation

@maocorte
Copy link
Contributor

What is this PR for?

Add an interpreter to work with a tachyon file system.
Properties required for the interpreter are:

  • tachyon master hostname
  • tachyon master port

What type of PR is it?

feature

Is there a relevant Jira issue?

https://issues.apache.org/jira/browse/ZEPPELIN-604

How should this be tested?

  • Install and configure Tachyon on a local machine.
  • run Tachyon in local mode $ ./bin/tachyon-start.sh local
  • check that interpreter params are setted to default values (hostname: localhost, port: 19998)
  • use the Tachyon CLI commands to interact with your Tachyon file system

Screenshots (if appropriate)

tachyon-interpreter

Questions:

  • Does the licenses files need update? no
  • Is there breaking changes for older versions? no
  • Does this needs documentation? no

/cc @jsimsa for the support he give us to develop this feature

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could we please log this in logger

@maocorte
Copy link
Contributor Author

Hi @felixcheung , many thanks for your suggestions.
I hope the changes are consisten with the request.

@felixcheung
Copy link
Member

thanks. and thanks for contributing this new interpreter!
In general we would prefer a new interpreter committed along with brief, introductory documentation.
Also the dependency of tachyon-shell we would need to add to license.
You could find an example of both with this PR #520

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not 100% sure but I believe it is required that any file would have the Apache License, even the embedded resource - like this one

Is this something you could accommodate?

@maocorte
Copy link
Contributor Author

Thank you for the suggestions, I hope changes will be fine

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of loop through keywords list, could keywords be in form of HashSet so you could just check HashSet.contains?
Does it need to check with startsWith?

@maocorte
Copy link
Contributor Author

Hi @hsaputra , thank you for your advices.
Sorry for the question, shouldent HashSet.contains fail if comparison is not equal between the values?
About your last advice, if contains is preferable to startsWith, I'll be happy to change it.
Thank you again.

@felixcheung
Copy link
Member

I think startsWith should be ok. I see that there are autocomplete doing substring matches as well. @hsaputra what do you think?

Also @maocorte would you be able to add a simple doc .md file and test to complete this interpreter
.md doc
Test

@maocorte
Copy link
Contributor Author

I've added the doc for the interpreter and a simple method to print the availble commands list.
I'm studying the better way to run a simple Tachyon cluster for the tests.
Thank you for your advices.

@jsimsa
Copy link
Contributor

jsimsa commented Jan 19, 2016

@maocorte if you need to spin up a test EC2 Tachyon cluster, the Tachyon project contains scripts for doing exactly that: http://tachyon-project.org/documentation/Running-Tachyon-on-EC2.html

@maocorte
Copy link
Contributor Author

Thank you @jsimsa, I was thinking about using LocalTachyonCluster to creare a local cluster and interact with it.
Do you think it could be a good solution?

@jsimsa
Copy link
Contributor

jsimsa commented Jan 19, 2016

That is an interesting idea. I know that Tachyon makes extensive use of it in its integration tests, but I am not sure if you will be able to interact with it from the Zeppelin interpreter. It is worth a try though and if it works, it would be definitely be a better way of testing the interpreter.

@hsaputra
Copy link

You are right, we could use startsWith. Was trying to avoid iterating through all keywords but using hash set not the right way. Maybe something like trie would work but would be too much for this.
+1 from me

@maocorte
Copy link
Contributor Author

I've added few tests for the interpreter using LocalTachyonCluster, any suggestion is appreciated.
Just a question: for test porpouse, I've added tachyon-servers, tachyon-minicluster and tachyon-underfs-local dependencies, must I add them into licences file?
Thank you.

@felixcheung
Copy link
Member

I'd believe so - they should be added to license even if it is test only

@maocorte
Copy link
Contributor Author

Done, thank you!!!

@felixcheung
Copy link
Member

I tried to test this but was not able to pull the changes from the branch, I'll look into this a bit more.

@maocorte
Copy link
Contributor Author

Hi @felixcheung , I have tried but works for me.
Please let me know if you need something from my side to merge this PR

@felixcheung
Copy link
Member

Tested, worked well, thanks!

One small question, some invalid commands do not always give an error message, do you know why?
image

Also for the doc, could you add a link from https://github.com/apache/incubator-zeppelin/blob/master/docs/_includes/themes/zeppelin/_navigation.html#L47

And could you please update the doc format to be consistent with this recent commit: #648

@maocorte
Copy link
Contributor Author

Changed format of tachyon documentation to new guidelines and added link to navigation page.
About interpreter response management, it reflects the behavior of tachyon tfs shell, in particular the method run.
Thanks for you suggestions.

@felixcheung
Copy link
Member

Thanks @AhyoungRyu could you please take a quick look at the doc too?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a small typo in here. Maybe localhost is right. isn't it?

@AhyoungRyu
Copy link
Contributor

Hi @maocorte : ) It's really good news that we have Tachyon interpreter !

How about adding a basic Tachyon example code and result image(if possible) to the How to test it's working section? If so, people can test and start Tachyon interpreter much easier.
Here is a good example: Scalding interpreter for Zeppelin.

Thanks!

@maocorte
Copy link
Contributor Author

Thank you @AhyoungRyu for your review and your suggestions and I'm glad you appreciate the idea of a Tachyon interpreter.
I have tried to add a simple example of using/testing the interpreter, using also the sh interpreter.
It is very simple but I hope it is significant and correctly formatted with the documentation guidelines.

@AhyoungRyu
Copy link
Contributor

@maocorte
Thank you for your quick response! Hopefully it will be : )
I applied this PR and found this.
screen shot 2016-01-27 at 10 52 35 pm

If you add a new line below line number 203 in tachyon.md, then you can see the bullets are working properly.
screen shot 2016-01-27 at 10 58 04 pm

@maocorte
Copy link
Contributor Author

Thank you @AhyoungRyu...sorry, my fault!!
I've used a bad markdown editor..

@felixcheung
Copy link
Member

thanks! merging if there is no more discussion

@asfgit asfgit closed this in 1b3784d Jan 28, 2016
@jsimsa
Copy link
Contributor

jsimsa commented Jan 29, 2016

@maocorte thanks for the PR!

@felixcheung OOC, when will the Tachyon documentation appear on the zeppelin website? (https://zeppelin.incubator.apache.org/docs/0.6.0-incubating-SNAPSHOT/ -> Interpreter)

@maocorte maocorte deleted the tachyon-interpreter branch March 29, 2016 10:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants