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/20 07:38:49 UTC
[GitHub] [pulsar] sijie commented on a change in pull request #9237: Separate the Functions validation and transforamtion
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