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:
[email protected]
