-
Notifications
You must be signed in to change notification settings - Fork 171
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Doc] Use absolute path when installing functions or using localrun #877
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall, just a few minor suggestions regarding the use of $PWD, and the exclusion of classname for NAR files.
@@ -46,7 +46,7 @@ pulsar-admin functions create \ | |||
--inputs test-input-topic \ | |||
--output persistent://public/default/test-output-topic \ | |||
--classname org.apache.pulsar.functions.api.examples.ExclamationFunction \ | |||
--jar /examples/api-examples.jar | |||
--jar $PWD/examples/api-examples.jar |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is $PWD a safe assumption as to the proper location of the artifact, or should we use a more explicit nomenclature such as path/to/examples/api-examples.jar
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think $PWD
gives a good hint that it's an absolute path. At least I have found it useful with pulsar-admin
when I have the files in the current directory and I want to reference the files with an absolute path. I guess there are also other ways. Perhaps it could be explained to the user that $PWD
is the current working directory. They will also find out if they try it or google it. :)
In this particular case, the original /examples/api-examples.jar
doesn't seem to make much sense. That's why I added the $PWD
prefix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The /path/to/file.jar
type of examples have the problem that for the user, it's really a lot of extra steps to find out the absolute path for the file arguments. That's why I prefer $PWD/file.jar
since it's simple and if someone doesn't already know it, they have the chance to learn it.
$PWD
is for a unix shell, but the examples are already assuming a unix shell since the syntax is all unix like.
LGTM |
Motivation
Absolute paths should be passed to pulsar-admin when creating functions/sinks/sources or when using localrun.
This should be explicitly mentioned in the documentation.
Additional context
Relative paths with pulsar-admin have never worked because of https://github.com/apache/pulsar/blob/50121e7f7be541f45bb6dc976f51e30658b1cb8d/bin/pulsar-admin#L26