You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2022/07/08 08:31:36 UTC
[GitHub] [pulsar] cbornet opened a new pull request, #16472: [cleanup][functions] Remove unused code
cbornet opened a new pull request, #16472:
URL: https://github.com/apache/pulsar/pull/16472
### Motivation
packageUrl is not set anymore in FunctionDetails so code that depend on it can be removed.
In the case of KubernetesRuntime we currently download the function itself and not the package.
This change doesn't modify this behaviour.
### Verifying this change
- [ ] Make sure that the change passes the CI checks.
This change is a trivial rework / code cleanup without any test coverage.
### Does this pull request potentially affect one of the following parts:
no
*If `yes` was chosen, please highlight the changes*
- Dependencies (does it add or upgrade a dependency): (yes / no)
- The public API: (yes / no)
- The schema: (yes / no / don't know)
- The default values of configurations: (yes / no)
- The wire protocol: (yes / no)
- The rest endpoints: (yes / no)
- The admin cli options: (yes / no)
- Anything that affects deployment: (yes / no / don't know)
### Documentation
Check the box below or label this PR directly.
Need to update docs?
- [ ] `doc-required`
(Your PR needs to update docs and you will update later)
- [x] `doc-not-needed`
cleanup
- [ ] `doc`
(Your PR contains doc changes)
- [ ] `doc-complete`
(Docs have been already added)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] cbornet commented on a diff in pull request #16472: [cleanup][functions] Remove unused code
Posted by GitBox <gi...@apache.org>.
cbornet commented on code in PR #16472:
URL: https://github.com/apache/pulsar/pull/16472#discussion_r924193993
##########
pulsar-functions/runtime/src/main/java/org/apache/pulsar/functions/runtime/kubernetes/KubernetesRuntime.java:
##########
@@ -855,13 +854,8 @@ protected List<String> getExecutorCommand() {
}
private List<String> getDownloadCommand(Function.FunctionDetails functionDetails, String userCodeFilePath) {
- if (Arrays.stream(PackageType.values()).anyMatch(type ->
Review Comment:
No `functions download` will [download the package](https://github.com/apache/pulsar/blob/172a84624f47bb722e6b419de4e239fbb8ecc154/pulsar-functions/worker/src/main/java/org/apache/pulsar/functions/worker/rest/api/ComponentImpl.java#L1483) when appropriate.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] cbornet commented on pull request #16472: [cleanup][functions] Remove unused code
Posted by GitBox <gi...@apache.org>.
cbornet commented on PR #16472:
URL: https://github.com/apache/pulsar/pull/16472#issuecomment-1188867486
/pulsarbot rerun-failure-checks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] cbornet commented on a diff in pull request #16472: [cleanup][functions] Remove unused code
Posted by GitBox <gi...@apache.org>.
cbornet commented on code in PR #16472:
URL: https://github.com/apache/pulsar/pull/16472#discussion_r930134978
##########
pulsar-functions/runtime/src/main/java/org/apache/pulsar/functions/runtime/kubernetes/KubernetesRuntime.java:
##########
@@ -855,13 +854,8 @@ protected List<String> getExecutorCommand() {
}
private List<String> getDownloadCommand(Function.FunctionDetails functionDetails, String userCodeFilePath) {
- if (Arrays.stream(PackageType.values()).anyMatch(type ->
Review Comment:
Sorry to be insistent but this code [downloads the package](https://github.com/apache/pulsar/blob/172a84624f47bb722e6b419de4e239fbb8ecc154/pulsar-functions/worker/src/main/java/org/apache/pulsar/functions/worker/rest/api/FunctionsImpl.java#L852) using the admin client as the `downloadPackageFile` name of the function suggests. It would be a bug if it were not so.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] zymap commented on a diff in pull request #16472: [cleanup][functions] Remove unused code
Posted by GitBox <gi...@apache.org>.
zymap commented on code in PR #16472:
URL: https://github.com/apache/pulsar/pull/16472#discussion_r917515892
##########
pulsar-functions/runtime/src/main/java/org/apache/pulsar/functions/runtime/kubernetes/KubernetesRuntime.java:
##########
@@ -855,13 +854,8 @@ protected List<String> getExecutorCommand() {
}
private List<String> getDownloadCommand(Function.FunctionDetails functionDetails, String userCodeFilePath) {
- if (Arrays.stream(PackageType.values()).anyMatch(type ->
Review Comment:
Actually, this download method is similar to line 867. The only difference is we save the function in a different place. We provide the [package management](https://docs.google.com/document/d/1FDSNhc8YB1yUMFBUrx8p-1nH1s5Kftp04PkFKWec-2U/edit) to save the functions. Should we fix the packageUrl at the register side not remove this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] cbornet commented on pull request #16472: [cleanup][functions] Remove unused code
Posted by GitBox <gi...@apache.org>.
cbornet commented on PR #16472:
URL: https://github.com/apache/pulsar/pull/16472#issuecomment-1178808168
cc @zymap
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] cbornet commented on a diff in pull request #16472: [cleanup][functions] Remove unused code
Posted by GitBox <gi...@apache.org>.
cbornet commented on code in PR #16472:
URL: https://github.com/apache/pulsar/pull/16472#discussion_r922667321
##########
pulsar-functions/runtime/src/main/java/org/apache/pulsar/functions/runtime/kubernetes/KubernetesRuntime.java:
##########
@@ -855,13 +854,8 @@ protected List<String> getExecutorCommand() {
}
private List<String> getDownloadCommand(Function.FunctionDetails functionDetails, String userCodeFilePath) {
- if (Arrays.stream(PackageType.values()).anyMatch(type ->
Review Comment:
Since we can download anyway the function itself directly, is there value to do it ?
Can this be discussed and done in another PR ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on a diff in pull request #16472: [cleanup][functions] Remove unused code
Posted by GitBox <gi...@apache.org>.
eolivelli commented on code in PR #16472:
URL: https://github.com/apache/pulsar/pull/16472#discussion_r926482611
##########
pulsar-functions/runtime/src/main/java/org/apache/pulsar/functions/runtime/kubernetes/KubernetesRuntime.java:
##########
@@ -855,13 +854,8 @@ protected List<String> getExecutorCommand() {
}
private List<String> getDownloadCommand(Function.FunctionDetails functionDetails, String userCodeFilePath) {
- if (Arrays.stream(PackageType.values()).anyMatch(type ->
Review Comment:
I think that "functions download" should work as expected, that is to download from DLog or from PackageManagement
do you have any pointers about the problem ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] cbornet commented on a diff in pull request #16472: [cleanup][functions] Remove unused code
Posted by GitBox <gi...@apache.org>.
cbornet commented on code in PR #16472:
URL: https://github.com/apache/pulsar/pull/16472#discussion_r930134978
##########
pulsar-functions/runtime/src/main/java/org/apache/pulsar/functions/runtime/kubernetes/KubernetesRuntime.java:
##########
@@ -855,13 +854,8 @@ protected List<String> getExecutorCommand() {
}
private List<String> getDownloadCommand(Function.FunctionDetails functionDetails, String userCodeFilePath) {
- if (Arrays.stream(PackageType.values()).anyMatch(type ->
Review Comment:
Sorry to be insistent but this code [downloads the package](https://github.com/apache/pulsar/blob/172a84624f47bb722e6b419de4e239fbb8ecc154/pulsar-functions/worker/src/main/java/org/apache/pulsar/functions/worker/rest/api/FunctionsImpl.java#L852) using the admin client as the `downloadPackageFile` name of the method suggests. It is then returned in the StreamingOutput result of the downloadFunction HTTP endpoint. It would be a bug if it were not so.
We don't need `packageUrl` because we have the package path in `packageLocation` of the `FunctionMetadata` which is what is used here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] cbornet commented on pull request #16472: [cleanup][functions] Remove unused code
Posted by GitBox <gi...@apache.org>.
cbornet commented on PR #16472:
URL: https://github.com/apache/pulsar/pull/16472#issuecomment-1179081170
/pulsarbot rerun-failure-checks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] zymap commented on a diff in pull request #16472: [cleanup][functions] Remove unused code
Posted by GitBox <gi...@apache.org>.
zymap commented on code in PR #16472:
URL: https://github.com/apache/pulsar/pull/16472#discussion_r929431399
##########
pulsar-functions/runtime/src/main/java/org/apache/pulsar/functions/runtime/kubernetes/KubernetesRuntime.java:
##########
@@ -855,13 +854,8 @@ protected List<String> getExecutorCommand() {
}
private List<String> getDownloadCommand(Function.FunctionDetails functionDetails, String userCodeFilePath) {
- if (Arrays.stream(PackageType.values()).anyMatch(type ->
Review Comment:
IIRC, when you run this in Kubernetes, the pod needs to download the function from the function worker. This [place](https://github.com/apache/pulsar/blob/172a84624f47bb722e6b419de4e239fbb8ecc154/pulsar-functions/worker/src/main/java/org/apache/pulsar/functions/worker/rest/api/ComponentImpl.java#L1481) only download the function in the function worker and do some validates. So the download command will run in the function instance and then start the instance.
So my intention is we should add the packageUrl when the function is uploading by a package management URL. So the Kubernetes runtime can get the function package from the package management service, not from the function worker.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] cbornet commented on a diff in pull request #16472: [cleanup][functions] Remove unused code
Posted by GitBox <gi...@apache.org>.
cbornet commented on code in PR #16472:
URL: https://github.com/apache/pulsar/pull/16472#discussion_r924193993
##########
pulsar-functions/runtime/src/main/java/org/apache/pulsar/functions/runtime/kubernetes/KubernetesRuntime.java:
##########
@@ -855,13 +854,8 @@ protected List<String> getExecutorCommand() {
}
private List<String> getDownloadCommand(Function.FunctionDetails functionDetails, String userCodeFilePath) {
- if (Arrays.stream(PackageType.values()).anyMatch(type ->
Review Comment:
> IIRC, the functions download will go for the files stored in Bookkeeper DLog only, and will not download packages from the package management service.
No, it's not from BookKeeper, `functions download` will [download the package](https://github.com/apache/pulsar/blob/172a84624f47bb722e6b419de4e239fbb8ecc154/pulsar-functions/worker/src/main/java/org/apache/pulsar/functions/worker/rest/api/ComponentImpl.java#L1481) when appropriate.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] freeznet commented on a diff in pull request #16472: [cleanup][functions] Remove unused code
Posted by GitBox <gi...@apache.org>.
freeznet commented on code in PR #16472:
URL: https://github.com/apache/pulsar/pull/16472#discussion_r923233394
##########
pulsar-functions/runtime/src/main/java/org/apache/pulsar/functions/runtime/kubernetes/KubernetesRuntime.java:
##########
@@ -855,13 +854,8 @@ protected List<String> getExecutorCommand() {
}
private List<String> getDownloadCommand(Function.FunctionDetails functionDetails, String userCodeFilePath) {
- if (Arrays.stream(PackageType.values()).anyMatch(type ->
Review Comment:
@cbornet the package management service has versioning on packages, and it can support different storage backend other than bookkeeper DLog.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] cbornet commented on a diff in pull request #16472: [cleanup][functions] Remove unused code
Posted by GitBox <gi...@apache.org>.
cbornet commented on code in PR #16472:
URL: https://github.com/apache/pulsar/pull/16472#discussion_r924193993
##########
pulsar-functions/runtime/src/main/java/org/apache/pulsar/functions/runtime/kubernetes/KubernetesRuntime.java:
##########
@@ -855,13 +854,8 @@ protected List<String> getExecutorCommand() {
}
private List<String> getDownloadCommand(Function.FunctionDetails functionDetails, String userCodeFilePath) {
- if (Arrays.stream(PackageType.values()).anyMatch(type ->
Review Comment:
> IIRC, the functions download will go for the files stored in Bookkeeper DLog only, and will not download packages from the package management service.
No `functions download` will [download the package](https://github.com/apache/pulsar/blob/172a84624f47bb722e6b419de4e239fbb8ecc154/pulsar-functions/worker/src/main/java/org/apache/pulsar/functions/worker/rest/api/ComponentImpl.java#L1483) when appropriate.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] cbornet commented on pull request #16472: [cleanup][functions] Remove unused code
Posted by GitBox <gi...@apache.org>.
cbornet commented on PR #16472:
URL: https://github.com/apache/pulsar/pull/16472#issuecomment-1189311925
/pulsarbot rerun-failure-checks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] zymap commented on pull request #16472: [cleanup][functions] Remove unused code
Posted by GitBox <gi...@apache.org>.
zymap commented on PR #16472:
URL: https://github.com/apache/pulsar/pull/16472#issuecomment-1179928140
@nlu90 @freeznet Would like to know your thought
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] cbornet commented on a diff in pull request #16472: [cleanup][functions] Remove unused code
Posted by GitBox <gi...@apache.org>.
cbornet commented on code in PR #16472:
URL: https://github.com/apache/pulsar/pull/16472#discussion_r930134978
##########
pulsar-functions/runtime/src/main/java/org/apache/pulsar/functions/runtime/kubernetes/KubernetesRuntime.java:
##########
@@ -855,13 +854,8 @@ protected List<String> getExecutorCommand() {
}
private List<String> getDownloadCommand(Function.FunctionDetails functionDetails, String userCodeFilePath) {
- if (Arrays.stream(PackageType.values()).anyMatch(type ->
Review Comment:
Sorry to be insistent but this code [downloads the package](https://github.com/apache/pulsar/blob/172a84624f47bb722e6b419de4e239fbb8ecc154/pulsar-functions/worker/src/main/java/org/apache/pulsar/functions/worker/rest/api/FunctionsImpl.java#L852) using the admin client as the `downloadPackageFile` name of the method suggests. It would be a bug if it were not so.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] cbornet commented on a diff in pull request #16472: [cleanup][functions] Remove unused code
Posted by GitBox <gi...@apache.org>.
cbornet commented on code in PR #16472:
URL: https://github.com/apache/pulsar/pull/16472#discussion_r924193993
##########
pulsar-functions/runtime/src/main/java/org/apache/pulsar/functions/runtime/kubernetes/KubernetesRuntime.java:
##########
@@ -855,13 +854,8 @@ protected List<String> getExecutorCommand() {
}
private List<String> getDownloadCommand(Function.FunctionDetails functionDetails, String userCodeFilePath) {
- if (Arrays.stream(PackageType.values()).anyMatch(type ->
Review Comment:
> IIRC, the functions download will go for the files stored in Bookkeeper DLog only, and will not download packages from the package management service.
No `functions download` will [download the package](https://github.com/apache/pulsar/blob/172a84624f47bb722e6b419de4e239fbb8ecc154/pulsar-functions/worker/src/main/java/org/apache/pulsar/functions/worker/rest/api/ComponentImpl.java#L1481) when appropriate.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] cbornet commented on a diff in pull request #16472: [cleanup][functions] Remove unused code
Posted by GitBox <gi...@apache.org>.
cbornet commented on code in PR #16472:
URL: https://github.com/apache/pulsar/pull/16472#discussion_r928937790
##########
pulsar-functions/runtime/src/main/java/org/apache/pulsar/functions/runtime/kubernetes/KubernetesRuntime.java:
##########
@@ -855,13 +854,8 @@ protected List<String> getExecutorCommand() {
}
private List<String> getDownloadCommand(Function.FunctionDetails functionDetails, String userCodeFilePath) {
- if (Arrays.stream(PackageType.values()).anyMatch(type ->
Review Comment:
@freeznet @zyma considering that `function download` downloads the package and not from BK when needed, do you still have issue with this PR ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on pull request #16472: [cleanup][functions] Remove unused code
Posted by GitBox <gi...@apache.org>.
eolivelli commented on PR #16472:
URL: https://github.com/apache/pulsar/pull/16472#issuecomment-1216448016
@zymap can we merge this patch ?
@freeznet approved
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli merged pull request #16472: [cleanup][functions] Remove unused code
Posted by GitBox <gi...@apache.org>.
eolivelli merged PR #16472:
URL: https://github.com/apache/pulsar/pull/16472
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] freeznet commented on a diff in pull request #16472: [cleanup][functions] Remove unused code
Posted by GitBox <gi...@apache.org>.
freeznet commented on code in PR #16472:
URL: https://github.com/apache/pulsar/pull/16472#discussion_r922933137
##########
pulsar-functions/runtime/src/main/java/org/apache/pulsar/functions/runtime/kubernetes/KubernetesRuntime.java:
##########
@@ -855,13 +854,8 @@ protected List<String> getExecutorCommand() {
}
private List<String> getDownloadCommand(Function.FunctionDetails functionDetails, String userCodeFilePath) {
- if (Arrays.stream(PackageType.values()).anyMatch(type ->
Review Comment:
Agreed with @zymap, the real issue here is the `packageUrl` is not been set properly, but not the "unused" issue. The correct way is to make the `packageUrl` set when the user creates functions with package URLs from the package management service.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] cbornet commented on a diff in pull request #16472: [cleanup][functions] Remove unused code
Posted by GitBox <gi...@apache.org>.
cbornet commented on code in PR #16472:
URL: https://github.com/apache/pulsar/pull/16472#discussion_r923056424
##########
pulsar-functions/runtime/src/main/java/org/apache/pulsar/functions/runtime/kubernetes/KubernetesRuntime.java:
##########
@@ -855,13 +854,8 @@ protected List<String> getExecutorCommand() {
}
private List<String> getDownloadCommand(Function.FunctionDetails functionDetails, String userCodeFilePath) {
- if (Arrays.stream(PackageType.values()).anyMatch(type ->
Review Comment:
What's the advantage compared to downloading the function ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] cbornet commented on a diff in pull request #16472: [cleanup][functions] Remove unused code
Posted by GitBox <gi...@apache.org>.
cbornet commented on code in PR #16472:
URL: https://github.com/apache/pulsar/pull/16472#discussion_r923529227
##########
pulsar-functions/runtime/src/main/java/org/apache/pulsar/functions/runtime/kubernetes/KubernetesRuntime.java:
##########
@@ -855,13 +854,8 @@ protected List<String> getExecutorCommand() {
}
private List<String> getDownloadCommand(Function.FunctionDetails functionDetails, String userCodeFilePath) {
- if (Arrays.stream(PackageType.values()).anyMatch(type ->
Review Comment:
I see the value of package management.
But in the context of downloading the code for a given Function, I don't see how using the CLI `functions download` command would return anything different than what is needed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] freeznet commented on a diff in pull request #16472: [cleanup][functions] Remove unused code
Posted by GitBox <gi...@apache.org>.
freeznet commented on code in PR #16472:
URL: https://github.com/apache/pulsar/pull/16472#discussion_r924126830
##########
pulsar-functions/runtime/src/main/java/org/apache/pulsar/functions/runtime/kubernetes/KubernetesRuntime.java:
##########
@@ -855,13 +854,8 @@ protected List<String> getExecutorCommand() {
}
private List<String> getDownloadCommand(Function.FunctionDetails functionDetails, String userCodeFilePath) {
- if (Arrays.stream(PackageType.values()).anyMatch(type ->
Review Comment:
IIRC, the `functions download` will go for the files stored in Bookkeeper DLog only, and will not download packages from the package management service. So if the user provides `packageUrl`, it should use `pulsar-admin packages download`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] cbornet commented on a diff in pull request #16472: [cleanup][functions] Remove unused code
Posted by GitBox <gi...@apache.org>.
cbornet commented on code in PR #16472:
URL: https://github.com/apache/pulsar/pull/16472#discussion_r924193993
##########
pulsar-functions/runtime/src/main/java/org/apache/pulsar/functions/runtime/kubernetes/KubernetesRuntime.java:
##########
@@ -855,13 +854,8 @@ protected List<String> getExecutorCommand() {
}
private List<String> getDownloadCommand(Function.FunctionDetails functionDetails, String userCodeFilePath) {
- if (Arrays.stream(PackageType.values()).anyMatch(type ->
Review Comment:
> IIRC, the functions download will go for the files stored in Bookkeeper DLog only, and will not download packages from the package management service.
No `functions download` will [download the package](https://github.com/apache/pulsar/blob/172a84624f47bb722e6b419de4e239fbb8ecc154/pulsar-functions/worker/src/main/java/org/apache/pulsar/functions/worker/rest/api/ComponentImpl.java#L1481-L1483) when appropriate.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] cbornet commented on a diff in pull request #16472: [cleanup][functions] Remove unused code
Posted by GitBox <gi...@apache.org>.
cbornet commented on code in PR #16472:
URL: https://github.com/apache/pulsar/pull/16472#discussion_r930134978
##########
pulsar-functions/runtime/src/main/java/org/apache/pulsar/functions/runtime/kubernetes/KubernetesRuntime.java:
##########
@@ -855,13 +854,8 @@ protected List<String> getExecutorCommand() {
}
private List<String> getDownloadCommand(Function.FunctionDetails functionDetails, String userCodeFilePath) {
- if (Arrays.stream(PackageType.values()).anyMatch(type ->
Review Comment:
Sorry to be insistent but this code [downloads the package](https://github.com/apache/pulsar/blob/172a84624f47bb722e6b419de4e239fbb8ecc154/pulsar-functions/worker/src/main/java/org/apache/pulsar/functions/worker/rest/api/FunctionsImpl.java#L852) using the admin client as the `downloadPackageFile` name of the method suggests. It is then returned in the StreamingOutput result of the downloadFunction HTTP endpoint. It would be a bug if it were not so.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org