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 2021/01/19 13:03:42 UTC
[GitHub] [pulsar] zymap opened a new pull request #9237: Separate the Functions validation and transforamtion
zymap opened a new pull request #9237:
URL: https://github.com/apache/pulsar/pull/9237
---
Fixes #9180
*Motivation*
When creating a function, we will do the validation for the
function package and transform the FunctionConfig to the
FunctionDetail in the same method.
We introduced package management for saving the function
packages, and want to reduce the duplicated validation when
creating a function with a package name.
So I suggest we need to separate the validation steps and
transformation step to make the method more clear.
### Verifying this change
Because we only refine the code and doesn't change the logic, so we need to make sure the existent CI passed.
- [x] Make sure that the change passes the CI 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] zymap commented on pull request #9237: Separate the Functions validation and transforamtion
Posted by GitBox <gi...@apache.org>.
zymap commented on pull request #9237:
URL: https://github.com/apache/pulsar/pull/9237#issuecomment-767361679
/pulsarbot run-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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] jerrypeng commented on a change in pull request #9237: Separate the Functions validation and transforamtion
Posted by GitBox <gi...@apache.org>.
jerrypeng commented on a change in pull request #9237:
URL: https://github.com/apache/pulsar/pull/9237#discussion_r569049716
##########
File path: pulsar-functions/worker/src/main/java/org/apache/pulsar/functions/worker/rest/api/FunctionsImpl.java
##########
@@ -754,17 +721,19 @@ public void updateFunctionOnWorkerLeader(final String tenant,
else{
clsLoader = FunctionConfigUtils.validate(functionConfig, componentPackageFile);
}
- return FunctionConfigUtils.convert(functionConfig, clsLoader);
-
- }
-
- private static boolean hasPackageTypePrefix(String destPkgUrl) {
- return Arrays.stream(PackageType.values()).anyMatch(type -> destPkgUrl.startsWith(type.toString()));
+ return clsLoader;
}
- private File downloadPackageFile(String packageName) throws IOException, PulsarAdminException {
- File file = Files.createTempFile("function", ".tmp").toFile();
- worker().getBrokerAdmin().packages().download(packageName, file.toString());
- return file;
+ private ImmutablePair<String, String> getTypeClassName(String functionPackageUrl) {
+ try {
+ Map<String, String> properties = worker().getBrokerAdmin().packages()
Review comment:
I don't understand what we are doing here. We are not using pulsar admin to upload packages.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] sijie commented on a change in pull request #9237: Separate the Functions validation and transforamtion
Posted by GitBox <gi...@apache.org>.
sijie commented on a change in pull request #9237:
URL: https://github.com/apache/pulsar/pull/9237#discussion_r560730504
##########
File path: pulsar-functions/utils/src/main/java/org/apache/pulsar/functions/utils/FunctionConfigUtils.java
##########
@@ -941,4 +948,16 @@ public static FunctionConfig validateUpdate(FunctionConfig existingConfig, Funct
}
return mergedConfig;
}
+
+ public static boolean isPackageNameStyle(String functionPkgUrl) {
Review comment:
```suggestion
public static boolean isPulsarPackageUrl(String functionPkgUrl) {
```
##########
File path: pulsar-functions/worker/src/main/java/org/apache/pulsar/functions/worker/rest/api/FunctionsImpl.java
##########
@@ -89,101 +90,31 @@ public void registerFunction(final String tenant,
throwUnavailableException();
}
- if (tenant == null) {
- throw new RestException(Response.Status.BAD_REQUEST, "Tenant is not provided");
- }
- if (namespace == null) {
- throw new RestException(Response.Status.BAD_REQUEST, "Namespace is not provided");
- }
- if (functionName == null) {
- throw new RestException(Response.Status.BAD_REQUEST, "Function name is not provided");
- }
- if (functionConfig == null) {
- throw new RestException(Response.Status.BAD_REQUEST, "Function config is not provided");
- }
-
- try {
- if (!isAuthorizedRole(tenant, namespace, clientRole, clientAuthenticationDataHttps)) {
- log.error("{}/{}/{} Client [{}] is not authorized to register {}", tenant, namespace,
- functionName, clientRole, ComponentTypeUtils.toString(componentType));
- throw new RestException(Response.Status.UNAUTHORIZED, "client is not authorize to perform operation");
- }
- } catch (PulsarAdminException e) {
- log.error("{}/{}/{} Failed to authorize [{}]", tenant, namespace, functionName, e);
- throw new RestException(Response.Status.INTERNAL_SERVER_ERROR, e.getMessage());
- }
-
- try {
- // Check tenant exists
- worker().getBrokerAdmin().tenants().getTenantInfo(tenant);
-
- String qualifiedNamespace = tenant + "/" + namespace;
- List<String> namespaces = worker().getBrokerAdmin().namespaces().getNamespaces(tenant);
- if (namespaces != null && !namespaces.contains(qualifiedNamespace)) {
- String qualifiedNamespaceWithCluster = String.format("%s/%s/%s", tenant,
- worker().getWorkerConfig().getPulsarFunctionsCluster(), namespace);
- if (namespaces != null && !namespaces.contains(qualifiedNamespaceWithCluster)) {
- log.error("{}/{}/{} Namespace {} does not exist", tenant, namespace, functionName, namespace);
- throw new RestException(Response.Status.BAD_REQUEST, "Namespace does not exist");
- }
- }
- } catch (PulsarAdminException.NotAuthorizedException e) {
- log.error("{}/{}/{} Client [{}] is not authorized to operate {} on tenant", tenant, namespace,
- functionName, clientRole, ComponentTypeUtils.toString(componentType));
- throw new RestException(Response.Status.UNAUTHORIZED, "client is not authorize to perform operation");
- } catch (PulsarAdminException.NotFoundException e) {
- log.error("{}/{}/{} Tenant {} does not exist", tenant, namespace, functionName, tenant);
- throw new RestException(Response.Status.BAD_REQUEST, "Tenant does not exist");
- } catch (PulsarAdminException e) {
- log.error("{}/{}/{} Issues getting tenant data", tenant, namespace, functionName, e);
- throw new RestException(Response.Status.INTERNAL_SERVER_ERROR, e.getMessage());
- }
+ requestParamsPreCheck(tenant, namespace, functionName, functionConfig);
Review comment:
```suggestion
preCheckRequestParams(tenant, namespace, functionName, functionConfig);
```
##########
File path: pulsar-functions/worker/src/main/java/org/apache/pulsar/functions/worker/rest/api/FunctionsImpl.java
##########
@@ -722,20 +581,101 @@ public void updateFunctionOnWorkerLeader(final String tenant,
}
}
- private Function.FunctionDetails validateUpdateRequestParams(final String tenant,
- final String namespace,
- final String componentName,
- final FunctionConfig functionConfig,
- final File componentPackageFile) throws IOException {
+ private void requestParamsPreCheck(String tenant, String namespace, String functionName, FunctionConfig functionConfig) {
+ requestParamsPreCheck(tenant, namespace, functionName);
+ if (functionConfig == null) {
+ throw new RestException(Response.Status.BAD_REQUEST, "Function config is not provided");
+ }
+ }
+
+ private void requestParamsPreCheck(String tenant, String namespace, String functionName) {
+ if (tenant == null) {
+ throw new RestException(Response.Status.BAD_REQUEST, "Tenant is not provided");
+ }
+ if (namespace == null) {
+ throw new RestException(Response.Status.BAD_REQUEST, "Namespace is not provided");
+ }
+ if (functionName == null) {
+ throw new RestException(Response.Status.BAD_REQUEST, "Function name is not provided");
+ }
+ }
+
+ // validate the operating tenant exists and has proper permissions to operate
+ private void validateFunctionNamespaceOperation(String tenant, String namespace, String functionName,
Review comment:
You reverse the order of the original code. The original code checks `isAuhtorizedRole` first. Please don't change any code when you refactor code.
##########
File path: pulsar-functions/worker/src/main/java/org/apache/pulsar/functions/worker/rest/api/FunctionsImpl.java
##########
@@ -318,57 +227,16 @@ public void updateFunction(final String tenant,
Function.FunctionDetails functionDetails = null;
File componentPackageFile = null;
- try {
-
- // validate parameters
- try {
- if (isNotBlank(functionPkgUrl)) {
- if (hasPackageTypePrefix(functionPkgUrl)) {
- componentPackageFile = downloadPackageFile(functionName);
- } else {
- try {
- componentPackageFile = FunctionCommon.extractFileFromPkgURL(functionPkgUrl);
- } catch (Exception e) {
- throw new IllegalArgumentException(String.format("Encountered error \"%s\" when getting %s package from %s", e.getMessage(), ComponentTypeUtils.toString(componentType), functionPkgUrl));
- }
- }
- functionDetails = validateUpdateRequestParams(tenant, namespace, functionName,
- mergedConfig, componentPackageFile);
-
- } else if (existingComponent.getPackageLocation().getPackagePath().startsWith(Utils.FILE)
- || existingComponent.getPackageLocation().getPackagePath().startsWith(Utils.HTTP)) {
- try {
- componentPackageFile = FunctionCommon.extractFileFromPkgURL(existingComponent.getPackageLocation().getPackagePath());
- } catch (Exception e) {
- throw new IllegalArgumentException(String.format("Encountered error \"%s\" when getting %s package from %s", e.getMessage(), ComponentTypeUtils.toString(componentType), functionPkgUrl));
- }
- functionDetails = validateUpdateRequestParams(tenant, namespace, functionName,
- mergedConfig, componentPackageFile);
- } else if (uploadedInputStream != null) {
-
- componentPackageFile = WorkerUtils.dumpToTmpFile(uploadedInputStream);
- functionDetails = validateUpdateRequestParams(tenant, namespace, functionName,
- mergedConfig, componentPackageFile);
-
- } else if (existingComponent.getPackageLocation().getPackagePath().startsWith(Utils.BUILTIN)) {
- functionDetails = validateUpdateRequestParams(tenant, namespace, functionName,
- mergedConfig, componentPackageFile);
- if (!isFunctionCodeBuiltin(functionDetails) && (componentPackageFile == null || fileDetail == null)) {
- throw new IllegalArgumentException(ComponentTypeUtils.toString(componentType) + " Package is not provided");
- }
- } else {
-
- componentPackageFile = FunctionCommon.createPkgTempFile();
- componentPackageFile.deleteOnExit();
- WorkerUtils.downloadFromBookkeeper(worker().getDlogNamespace(), componentPackageFile, existingComponent.getPackageLocation().getPackagePath());
+ if (FunctionConfigUtils.isPackageNameStyle(functionPkgUrl)) {
Review comment:
In the original code, there is a logic verifying `existingComponent`. After refactor, the code doesn't exist there. It is very hard for reviewers to review this code and understand the change.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] zymap commented on a change in pull request #9237: Separate the Functions validation and transforamtion
Posted by GitBox <gi...@apache.org>.
zymap commented on a change in pull request #9237:
URL: https://github.com/apache/pulsar/pull/9237#discussion_r597447256
##########
File path: pulsar-functions/worker/src/main/java/org/apache/pulsar/functions/worker/rest/api/FunctionsImpl.java
##########
@@ -754,17 +721,19 @@ public void updateFunctionOnWorkerLeader(final String tenant,
else{
clsLoader = FunctionConfigUtils.validate(functionConfig, componentPackageFile);
}
- return FunctionConfigUtils.convert(functionConfig, clsLoader);
-
- }
-
- private static boolean hasPackageTypePrefix(String destPkgUrl) {
- return Arrays.stream(PackageType.values()).anyMatch(type -> destPkgUrl.startsWith(type.toString()));
+ return clsLoader;
}
- private File downloadPackageFile(String packageName) throws IOException, PulsarAdminException {
- File file = Files.createTempFile("function", ".tmp").toFile();
- worker().getBrokerAdmin().packages().download(packageName, file.toString());
- return file;
+ private ImmutablePair<String, String> getTypeClassName(String functionPackageUrl) {
+ try {
+ Map<String, String> properties = worker().getBrokerAdmin().packages()
Review comment:
We introduce [package management](https://docs.google.com/document/d/1FDSNhc8YB1yUMFBUrx8p-1nH1s5Kftp04PkFKWec-2U/edit) to manage function jars.
We used to get the FunctionTypes by loading the jar and get it from the class. If we want to use a jar from the package management, we want to get it directly from the metadata and avoid download the jar at the broker.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] zymap closed pull request #9237: Separate the Functions validation and transforamtion
Posted by GitBox <gi...@apache.org>.
zymap closed pull request #9237:
URL: https://github.com/apache/pulsar/pull/9237
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] zymap commented on pull request #9237: Separate the Functions validation and transforamtion
Posted by GitBox <gi...@apache.org>.
zymap commented on pull request #9237:
URL: https://github.com/apache/pulsar/pull/9237#issuecomment-802605726
Because this has many CI failed. I will close this PR and reopen it when it is ready.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org