simonbence commented on code in PR #8726:
URL: https://github.com/apache/nifi/pull/8726#discussion_r1587603716
##########
nifi-framework-bundle/nifi-framework/nifi-framework-components/src/main/java/org/apache/nifi/groups/StandardProcessGroup.java:
##########
@@ -4022,7 +4027,7 @@ private VersionedFlowSynchronizationContext
createGroupSynchronizationContext(fi
}
@Override
- public void verifyCanSaveToFlowRegistry(final String registryId, final
String bucketId, final String flowId, final String saveAction) {
+ public void verifyCanSaveToFlowRegistry(final String registryId, final
String branch, final String bucketId, final String flowId, final String
saveAction) {
Review Comment:
Should not the newly created `FlowLocation` used? If I am correct, branch,
bucket id and flow id covers just the same set of info as `FlowLocation`s state
covers.
##########
nifi-extension-bundles/nifi-flow-registry-client-bundle/nifi-flow-registry-client-services/src/main/java/org/apache/nifi/registry/flow/NifiRegistryFlowRegistryClient.java:
##########
@@ -149,7 +149,7 @@ public boolean isStorageLocationApplicable(final
FlowRegistryClientConfiguration
}
@Override
- public Set<FlowRegistryBucket> getBuckets(final
FlowRegistryClientConfigurationContext context) throws FlowRegistryException,
IOException {
+ public Set<FlowRegistryBucket> getBuckets(final
FlowRegistryClientConfigurationContext context, final String branch) throws
FlowRegistryException, IOException {
Review Comment:
What is the expectation against requests for "non-existing" branches? (In
case of the NiFiRegistry this should be always the default and I think it is
planned to enforce this from UI but do we want to have server-side validation
and failure for cases with different branch or we want branch parameter
completely ignored for NiFiRegistry)
The question comes from that in `getFlowContents` we do not ignore it. It
could be shortcutted to be main but it is handled properly, which somewhat
different from the branch handling in other methods.
##########
nifi-api/src/main/java/org/apache/nifi/registry/flow/FlowRegistryClient.java:
##########
@@ -71,30 +74,82 @@ public interface FlowRegistryClient extends
ConfigurableComponent {
*/
boolean isStorageLocationApplicable(FlowRegistryClientConfigurationContext
context, String location);
+ /**
+ * Indicates if the registry supports branching.
+ *
+ * @param context Configuration context
+ * @return true if the registry supports branching, false otherwise
+ */
+ default boolean isBranchingSupported(final
FlowRegistryClientConfigurationContext context) {
+ return false;
+ }
+
+ /**
+ * Get the available branches. Should return at least one branch that
matches the response of getDefaultBranch.
+ *
+ * @param context Configuration context
+ * @return the set of available branches
+ *
+ * @throws FlowRegistryException If an issue happens during processing the
request.
+ * @throws IOException If there is issue with the communication between
NiFi and the Flow Registry.
+ */
+ default Set<FlowRegistryBranch> getBranches(final
FlowRegistryClientConfigurationContext context) throws FlowRegistryException,
IOException {
+ return Set.of(getDefaultBranch(context));
+ }
+
+ /**
+ * Gets the default branch. Must return a non-null FlowRegistryBranch
instance with a non-null name.
Review Comment:
I think it might worth mentioning that in case of the impementation does not
support branching, a default value will be returned for convinience.
##########
nifi-api/src/main/java/org/apache/nifi/registry/flow/CreateBranch.java:
##########
@@ -0,0 +1,53 @@
+/*
+ *
+ * * Licensed to the Apache Software Foundation (ASF) under one or more
+ * * contributor license agreements. See the NOTICE file distributed with
+ * * this work for additional information regarding copyright ownership.
+ * * The ASF licenses this file to You under the Apache License, Version 2.0
+ * * (the "License"); you may not use this file except in compliance with
+ * * the License. You may obtain a copy of the License at
+ * *
+ * * http://www.apache.org/licenses/LICENSE-2.0
+ * *
+ * * Unless required by applicable law or agreed to in writing, software
+ * * distributed under the License is distributed on an "AS IS" BASIS,
+ * * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * * See the License for the specific language governing permissions and
+ * * limitations under the License.
+ *
+ */
+
+package org.apache.nifi.registry.flow;
+
+/**
+ * Information for creating a branch in a flow registry.
+ */
+public class CreateBranch {
+
+ private String fromBranch;
+ private String newBranch;
+
+ public CreateBranch() {
+ }
+
+ public CreateBranch(final String fromBranch, final String newBranch) {
+ this.fromBranch = fromBranch;
Review Comment:
When we do create a branch, is it expected to be branched to the current
"head" of the fromBranch or what is the expected behaviour?
##########
nifi-api/src/main/java/org/apache/nifi/registry/flow/FlowRegistryClient.java:
##########
@@ -71,30 +74,82 @@ public interface FlowRegistryClient extends
ConfigurableComponent {
*/
boolean isStorageLocationApplicable(FlowRegistryClientConfigurationContext
context, String location);
+ /**
+ * Indicates if the registry supports branching.
+ *
+ * @param context Configuration context
+ * @return true if the registry supports branching, false otherwise
+ */
+ default boolean isBranchingSupported(final
FlowRegistryClientConfigurationContext context) {
+ return false;
+ }
+
+ /**
+ * Get the available branches. Should return at least one branch that
matches the response of getDefaultBranch.
+ *
+ * @param context Configuration context
+ * @return the set of available branches
+ *
+ * @throws FlowRegistryException If an issue happens during processing the
request.
+ * @throws IOException If there is issue with the communication between
NiFi and the Flow Registry.
+ */
+ default Set<FlowRegistryBranch> getBranches(final
FlowRegistryClientConfigurationContext context) throws FlowRegistryException,
IOException {
+ return Set.of(getDefaultBranch(context));
+ }
+
+ /**
+ * Gets the default branch. Must return a non-null FlowRegistryBranch
instance with a non-null name.
+ *
+ * @param context Configuration context
+ * @return the default branch
+ *
+ * @throws FlowRegistryException If an issue happens during processing the
request.
+ * @throws IOException If there is issue with the communication between
NiFi and the Flow Registry.
+ */
+ default FlowRegistryBranch getDefaultBranch(final
FlowRegistryClientConfigurationContext context) throws FlowRegistryException,
IOException {
+ final FlowRegistryBranch branch = new FlowRegistryBranch();
+ branch.setName(DEFAULT_BRANCH_NAME);
+ return branch;
+ }
+
+ /**
+ * Creates a new branch from a given branch.
+ *
+ * @param context Configuration context
+ * @param createBranch the info for creating the branch
+ *
+ * @throws FlowRegistryException If an issue happens during processing the
request.
+ * @throws IOException If there is issue with the communication between
NiFi and the Flow Registry.
+ */
+ default void createBranch(FlowRegistryClientConfigurationContext context,
final CreateBranch createBranch) throws FlowRegistryException, IOException {
Review Comment:
Bucket creation is not supported in the client but spared for the actaul
registry service. Is not this out of the concept of the client as well?
##########
nifi-framework-bundle/nifi-framework/nifi-framework-components/src/main/java/org/apache/nifi/groups/StandardProcessGroup.java:
##########
@@ -4040,14 +4045,20 @@ public void verifyCanSaveToFlowRegistry(final String
registryId, final String bu
// Flow ID matches. We want to publish the Process Group as
the next version of the Flow, so we must
// ensure that all other parameters match as well.
+
+ if (branch != null && !Objects.equals(branch,
vci.getBranch())) {
Review Comment:
This two statement could be merged into one with the following way:
- `FlowLocation` might be able to proviede a `BucketLocation` instance (like
a partial path)
- We could compare `BucketLocation` instnaces
I think that would simplify this, might communicate the intent in a more
accurate manner and as we have the same error message we would not even loose
details
##########
nifi-api/src/main/java/org/apache/nifi/registry/flow/BucketLocation.java:
##########
@@ -0,0 +1,54 @@
+/*
+ *
+ * * Licensed to the Apache Software Foundation (ASF) under one or more
+ * * contributor license agreements. See the NOTICE file distributed with
+ * * this work for additional information regarding copyright ownership.
+ * * The ASF licenses this file to You under the Apache License, Version 2.0
+ * * (the "License"); you may not use this file except in compliance with
+ * * the License. You may obtain a copy of the License at
+ * *
+ * * http://www.apache.org/licenses/LICENSE-2.0
+ * *
+ * * Unless required by applicable law or agreed to in writing, software
+ * * distributed under the License is distributed on an "AS IS" BASIS,
+ * * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * * See the License for the specific language governing permissions and
+ * * limitations under the License.
+ *
+ */
+
+package org.apache.nifi.registry.flow;
+
+/**
+ * Information for locating a bucket in a flow registry.
+ */
+public class BucketLocation {
+
+ private String branch;
Review Comment:
Is there any contract or expectation against the relationship between branch
and bucket? Based on what I saw in the code, I assume the top level
organization is the branch, so buckts are _witnin_ the branches. (I might be
wrong, I do not remember seeing explicit mention of this) Is there any
expectations like a bucket must exists under every brunch (but the flows/flow
versions might differ)?
If this level structure stands, it can have a negative affect on the user
experience as buckets currently cannot be created from NiFi UI. So for example
I have a flow F1 within bucket B1 which are in the "main" branch. Assume I want
to move to a "dev" branch with a new version of F1. Multiple things might
happen depending on our purposes:
- If branches and buckets are ortogonal, the bucket will be available for
every branches
- If buckets (already existing under other branches) are created on the need
basis, registry implementations needs to know about this expectation and
support this
- If we expect buckets to be in place, currently the users needs to move to
the registry server beforehand and create the given bucket before committing.
I think based on the code I have a good educated guess about the idea but I
would find it beneficial to have a description about this somewhere (either in
documentation or javadoc, I am not sure)
##########
nifi-api/src/main/java/org/apache/nifi/registry/flow/FlowRegistryBranch.java:
##########
@@ -0,0 +1,52 @@
+/*
+ *
+ * * Licensed to the Apache Software Foundation (ASF) under one or more
+ * * contributor license agreements. See the NOTICE file distributed with
+ * * this work for additional information regarding copyright ownership.
+ * * The ASF licenses this file to You under the Apache License, Version 2.0
+ * * (the "License"); you may not use this file except in compliance with
+ * * the License. You may obtain a copy of the License at
+ * *
+ * * http://www.apache.org/licenses/LICENSE-2.0
+ * *
+ * * Unless required by applicable law or agreed to in writing, software
+ * * distributed under the License is distributed on an "AS IS" BASIS,
+ * * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * * See the License for the specific language governing permissions and
+ * * limitations under the License.
+ *
+ */
+
+package org.apache.nifi.registry.flow;
+
+import java.util.Objects;
+
+public class FlowRegistryBranch {
Review Comment:
Can it matter what is the parent branch? Or this is a detail to the registry
implementation and we do not wish to know?
##########
nifi-framework-bundle/nifi-framework/nifi-framework-components/src/main/java/org/apache/nifi/registry/flow/StandardFlowRegistryClientNode.java:
##########
@@ -369,19 +391,21 @@ private RegisteredFlowSnapshot fetchFlowContents(final
FlowRegistryClientUserCon
final boolean
fetchRemoteFlows) throws FlowRegistryException {
final String storageLocation = coordinates.getStorageLocation();
+ final String branch = coordinates.getBranch();
final String bucketId = coordinates.getBucketId();
final String flowId = coordinates.getFlowId();
final String version = coordinates.getVersion();
+ final FlowVersionLocation flowVersionLocation = new
FlowVersionLocation(branch, bucketId, flowId, version);
final List<FlowRegistryClientNode> clientNodes =
getRegistryClientsForInternalFlow(storageLocation);
for (final FlowRegistryClientNode clientNode : clientNodes) {
try {
- logger.debug("Attempting to fetch flow for Bucket [{}] Flow
[{}] Version [{}] using {}", bucketId, flowId, version, clientNode);
- final FlowSnapshotContainer snapshotContainer =
clientNode.getFlowContents(context, bucketId, flowId, version,
fetchRemoteFlows);
+ logger.debug("Attempting to fetch flow from branch [{}] for
Bucket [{}] Flow [{}] Version [{}] using {}", branch, bucketId, flowId,
version, clientNode);
+ final FlowSnapshotContainer snapshotContainer =
clientNode.getFlowContents(context, flowVersionLocation, fetchRemoteFlows);
final RegisteredFlowSnapshot snapshot =
snapshotContainer.getFlowSnapshot();
coordinates.setRegistryId(clientNode.getIdentifier());
- logger.debug("Successfully fetched flow for Bucket [{}] Flow
[{}] Version [{}] using {}", bucketId, flowId, version, clientNode);
+ logger.debug("Successfully fetched flow from branch [{}] for
Bucket [{}] Flow [{}] Version [{}] using {}", branch, bucketId, flowId,
version, clientNode);
Review Comment:
Minor: other parts of the location start with upper case. Should not branch
do that too?
##########
nifi-framework-bundle/nifi-framework/nifi-framework-components/src/main/java/org/apache/nifi/registry/flow/FlowAnalyzingRegistryClientNode.java:
##########
@@ -401,13 +401,33 @@ public boolean isStorageLocationApplicable(final String
location) {
}
@Override
- public Set<FlowRegistryBucket> getBuckets(final
FlowRegistryClientUserContext context) throws FlowRegistryException,
IOException {
- return node.getBuckets(context);
+ public boolean isBranchingSupported() {
+ return node.isBranchingSupported();
}
@Override
- public FlowRegistryBucket getBucket(final FlowRegistryClientUserContext
context, final String bucketId) throws FlowRegistryException, IOException {
- return node.getBucket(context, bucketId);
+ public Set<FlowRegistryBranch> getBranches(final
FlowRegistryClientUserContext context) throws FlowRegistryException,
IOException {
+ return node.getBranches(context);
+ }
+
+ @Override
+ public FlowRegistryBranch getDefaultBranch(final
FlowRegistryClientUserContext context) throws FlowRegistryException,
IOException {
+ return node.getDefaultBranch(context);
+ }
+
+ @Override
+ public void createBranch(final FlowRegistryClientUserContext context,
final CreateBranch createBranch) throws FlowRegistryException, IOException {
Review Comment:
Up to this point, the `FlowAnalyzingRegistryClientNode` did not have methods
with argumetns serving as "command object". In cases where it depended on more
complex structures, like in case of `RegisteredFlow`, these arguments were data
driven (here: `RegisteredFlow` and not `RegisterFlow`). I think this makes the
interface into somewhat inconsistent. I would suggest with providing two branch
representation, even if they are Strings or their own type (as I see other
methods go with String based representation, so if you go with their specific
type, I suggest to update those methods as well)
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
