You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by kevdoran <gi...@git.apache.org> on 2018/03/28 03:06:44 UTC
[GitHub] nifi-minifi pull request #120: MINIFI-448 C2 server functionality and intern...
GitHub user kevdoran opened a pull request:
https://github.com/apache/nifi-minifi/pull/120
MINIFI-448 C2 server functionality and internal interfaces
The C2 service layer is the part of the C2 framework that backs the REST API to provide the business logic.
This commit also defines a few preliminary provider interfaces for persistence based on what is required by the service layer. A reference, in-memory implementation of the provider interfaces is included.
---
Thank you for submitting a contribution to Apache NiFi - MiNiFi.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
### For all changes:
- [ ] Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
- [ ] Does your PR title start with MINIFI-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
- [ ] Has your PR been rebased against the latest commit within the target branch (typically master)?
- [ ] Is your initial contribution a single, squashed commit?
### For code changes:
- [ ] Have you ensured that the full suite of tests is executed via mvn -Pcontrib-check clean install at the root nifi-minifi folder?
- [ ] Have you written or updated unit tests to verify your changes?
- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
- [ ] If applicable, have you updated the LICENSE file, including the main LICENSE file under minifi-assembly?
- [ ] If applicable, have you updated the NOTICE file, including the main NOTICE file found under minifi-assembly?
### For documentation related changes:
- [ ] Have you ensured that format looks appropriate for the output in which it is rendered?
### Note:
Please ensure that once the PR is submitted, you check travis-ci for build issues and submit an update to your PR as soon as possible.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/kevdoran/nifi-minifi MINIFI-448-service-layer
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/nifi-minifi/pull/120.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #120
----
----
---
[GitHub] nifi-minifi pull request #120: MINIFI-448 C2 server functionality and intern...
Posted by bbende <gi...@git.apache.org>.
Github user bbende commented on a diff in the pull request:
https://github.com/apache/nifi-minifi/pull/120#discussion_r177766828
--- Diff: minifi-c2/pom.xml ---
@@ -37,10 +37,30 @@ limitations under the License.
<module>minifi-c2-framework</module>
<module>minifi-c2-integration-tests</module>
<module>minifi-c2-jetty</module>
+ <module>minifi-c2-provider-api</module>
<module>minifi-c2-web-api</module>
</modules>
+ <build>
+ <plugins>
+ <plugin>
+ <groupId>org.codehaus.gmavenplus</groupId>
+ <artifactId>gmavenplus-plugin</artifactId>
+ <version>1.5</version>
+ <executions>
+ <execution>
+ <goals>
+ <goal>addTestSources</goal>
+ <goal>testCompile</goal>
+ </goals>
+ </execution>
+ </executions>
+ </plugin>
+ </plugins>
+ </build>
+
<properties>
+ <nifi.registry.version>0.1.0</nifi.registry.version>
--- End diff --
Same property below
---
[GitHub] nifi-minifi pull request #120: MINIFI-448 C2 server functionality and intern...
Posted by bbende <gi...@git.apache.org>.
Github user bbende commented on a diff in the pull request:
https://github.com/apache/nifi-minifi/pull/120#discussion_r177765399
--- Diff: minifi-c2/minifi-c2-framework/src/main/java/org/apache/nifi/minifi/c2/core/service/StandardC2Service.java ---
@@ -0,0 +1,494 @@
+/*
+ * 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.minifi.c2.core.service;
+
+import org.apache.nifi.minifi.c2.api.provider.agent.AgentPersistenceProvider;
+import org.apache.nifi.minifi.c2.api.provider.device.DevicePersistenceProvider;
+import org.apache.nifi.minifi.c2.api.provider.operations.OperationPersistenceProvider;
+import org.apache.nifi.minifi.c2.core.exception.ResourceNotFoundException;
+import org.apache.nifi.minifi.c2.model.Agent;
+import org.apache.nifi.minifi.c2.model.AgentClass;
+import org.apache.nifi.minifi.c2.model.AgentManifest;
+import org.apache.nifi.minifi.c2.model.Device;
+import org.apache.nifi.minifi.c2.model.OperationRequest;
+import org.apache.nifi.minifi.c2.model.OperationState;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import org.springframework.beans.factory.annotation.Autowired;
+import org.springframework.stereotype.Service;
+
+import javax.validation.ConstraintViolation;
+import javax.validation.ConstraintViolationException;
+import javax.validation.Validator;
+import java.util.List;
+import java.util.Optional;
+import java.util.Set;
+import java.util.UUID;
+import java.util.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReentrantReadWriteLock;
+
+@Service
+public class StandardC2Service implements C2Service {
+
+ private static final Logger logger = LoggerFactory.getLogger(StandardC2Service.class);
+
+ private final AgentPersistenceProvider agentPersistenceProvider;
+ private final DevicePersistenceProvider devicePersistenceProvider;
+ private final OperationPersistenceProvider operationPersistenceProvider;
+ private final Validator validator;
+
+ private final ReentrantReadWriteLock lock = new ReentrantReadWriteLock();
--- End diff --
Do we think the read/write lock approach is too limiting for C2?
I know we did this in registry to ensure consistency when interacting with more than one data store, but the trade-off is that it reduces the concurrency a lot because all threads are blocked while one thread has the write lock.
I don't know the level of concurrent requests we are anticipating here so it may be a non-issue, but just wanted to throw it out there. This also may be totally fine for initial implementation, and we can modify/improve later.
I also haven't read through all the methods below yet, but if there isn't a case where a single method performs operations across multiple persistence providers, then we could probably make the locks more granular and have a lock for each persistence provider so that agent calls are not blocked by device calls, etc.
---
[GitHub] nifi-minifi pull request #120: MINIFI-448 C2 server functionality and intern...
Posted by kevdoran <gi...@git.apache.org>.
Github user kevdoran commented on a diff in the pull request:
https://github.com/apache/nifi-minifi/pull/120#discussion_r177852495
--- Diff: minifi-c2/minifi-c2-framework/src/test/groovy/org/apache/nifi/minifi/c2/core/service/StandardC2ProtocolServiceSpec.groovy ---
@@ -0,0 +1,164 @@
+/*
+ * 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.minifi.c2.core.service
+
+import org.apache.nifi.minifi.c2.api.provider.heartbeat.HeartbeatPersistenceProvider
+import org.apache.nifi.minifi.c2.model.*
+import spock.lang.Specification
+
+class StandardC2ProtocolServiceSpec extends Specification {
--- End diff --
Good question, @jzonthemtn. There is no technical/functional reason these tests couldn't be written as Java junit tests. The groovy framework I'm using here, [Spock](https://github.com/spockframework/spock), offers a few advantages:
- Detailed error messages for failed tests/assertions
- A reasonable terse/structured syntax that allows tests to serve as feature/method requirements/specifications. This was really beneficial in writing [StandardC2ServiceSpec](https://github.com/apache/nifi-minifi/pull/120/files#diff-f485f870b41384487a26ff065fd25467), which is extremely repetitive. I wanted the tests to be short and self-descriptive so I could keep track of what combinations were covered.
- Powerful mocking capabilities (with some advantages over Java libs such as Mockito)
- built-in test templating and data driven parameterization (although I did not use that feature in this test cases).
---
[GitHub] nifi-minifi pull request #120: MINIFI-448 C2 server functionality and intern...
Posted by kevdoran <gi...@git.apache.org>.
Github user kevdoran closed the pull request at:
https://github.com/apache/nifi-minifi/pull/120
---
[GitHub] nifi-minifi pull request #120: MINIFI-448 C2 server functionality and intern...
Posted by jzonthemtn <gi...@git.apache.org>.
Github user jzonthemtn commented on a diff in the pull request:
https://github.com/apache/nifi-minifi/pull/120#discussion_r177757827
--- Diff: minifi-c2/minifi-c2-framework/src/test/groovy/org/apache/nifi/minifi/c2/core/service/StandardC2ProtocolServiceSpec.groovy ---
@@ -0,0 +1,164 @@
+/*
+ * 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.minifi.c2.core.service
+
+import org.apache.nifi.minifi.c2.api.provider.heartbeat.HeartbeatPersistenceProvider
+import org.apache.nifi.minifi.c2.model.*
+import spock.lang.Specification
+
+class StandardC2ProtocolServiceSpec extends Specification {
--- End diff --
Just curious if there was a technical reason for using Groovy for the test? I don't know a whole lot about groovy so I could be missing something that's otherwise obvious.
---
[GitHub] nifi-minifi pull request #120: MINIFI-448 C2 server functionality and intern...
Posted by bbende <gi...@git.apache.org>.
Github user bbende commented on a diff in the pull request:
https://github.com/apache/nifi-minifi/pull/120#discussion_r177763765
--- Diff: minifi-c2/minifi-c2-framework/src/main/java/org/apache/nifi/minifi/c2/api/provider/device/DevicePersistenceProvider.java ---
@@ -0,0 +1,78 @@
+/*
+ * 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.minifi.c2.api.provider.device;
+
+import org.apache.nifi.minifi.c2.api.provider.Provider;
+import org.apache.nifi.minifi.c2.model.Device;
+
+import java.util.List;
+import java.util.Optional;
+
+/**
+ * NOTE: Although this interface is intended to be an extension point, it is not yet considered stable and thus may
+ * change across releases until the the C2 Server APIs mature.
+ *
+ * TODO, we may want to consider creating a separate entity model rather than reusing the REST API object model.
+ * Currently, this design assumes the Provider implementation will do that translation.
+ * This requires adding a dependency on minifi-c2-commons here for the data model.
+ */
+public interface DevicePersistenceProvider extends Provider {
--- End diff --
Would it make sense to have a base interface kind of like the Spring Data repositories, something like:
```
interface PersistenceProvider<T> {
long getCount();
T save(T t);
List<T> getAll();
Optional<T> get(String id);
void delete(T t);
}
```
Then some of the interfaces just become empty:
```
interface DevicePersistenceProvider extends PersistenceProvider<Device> {
// add any specific methods that only make sense for devices
}
```
The only case where it doesn't fully work is the AgentPersistenceProvider which has the CRUD operations for multiple domain objects in a single interface, which makes sense to enforce that they are persisted to the same provider.
Maybe that interface could provider sub-services for each domain type?
```
interface AgentPersistenceProvider {
PersistenceProvider<AgentClass> getAgentClassPersistenceProvider();
PersistenceProvider<AgentManifest> getAgentManifestPersistenceProvider();
PersistenceProvider<Agent> getAgentPersistenceProvider();
}
```
So the only pluggable provider is the top-level AgentPersistenceProvider, but then it is up to that provider to provider implementations of the sub-providers.
Feel free to tell me this makes no sense, just throwing out ideas.
---
[GitHub] nifi-minifi pull request #120: MINIFI-448 C2 server functionality and intern...
Posted by kevdoran <gi...@git.apache.org>.
Github user kevdoran commented on a diff in the pull request:
https://github.com/apache/nifi-minifi/pull/120#discussion_r177827045
--- Diff: minifi-c2/minifi-c2-framework/src/main/java/org/apache/nifi/minifi/c2/core/service/StandardC2Service.java ---
@@ -0,0 +1,494 @@
+/*
+ * 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.minifi.c2.core.service;
+
+import org.apache.nifi.minifi.c2.api.provider.agent.AgentPersistenceProvider;
+import org.apache.nifi.minifi.c2.api.provider.device.DevicePersistenceProvider;
+import org.apache.nifi.minifi.c2.api.provider.operations.OperationPersistenceProvider;
+import org.apache.nifi.minifi.c2.core.exception.ResourceNotFoundException;
+import org.apache.nifi.minifi.c2.model.Agent;
+import org.apache.nifi.minifi.c2.model.AgentClass;
+import org.apache.nifi.minifi.c2.model.AgentManifest;
+import org.apache.nifi.minifi.c2.model.Device;
+import org.apache.nifi.minifi.c2.model.OperationRequest;
+import org.apache.nifi.minifi.c2.model.OperationState;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import org.springframework.beans.factory.annotation.Autowired;
+import org.springframework.stereotype.Service;
+
+import javax.validation.ConstraintViolation;
+import javax.validation.ConstraintViolationException;
+import javax.validation.Validator;
+import java.util.List;
+import java.util.Optional;
+import java.util.Set;
+import java.util.UUID;
+import java.util.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReentrantReadWriteLock;
+
+@Service
+public class StandardC2Service implements C2Service {
+
+ private static final Logger logger = LoggerFactory.getLogger(StandardC2Service.class);
+
+ private final AgentPersistenceProvider agentPersistenceProvider;
+ private final DevicePersistenceProvider devicePersistenceProvider;
+ private final OperationPersistenceProvider operationPersistenceProvider;
+ private final Validator validator;
+
+ private final ReentrantReadWriteLock lock = new ReentrantReadWriteLock();
--- End diff --
Great points, @bbende. Thanks for the input.
This is probably too limiting as you point. One of the things I like about trying to handle concurrency in the service layer, as we did for registry, is that there is less burden placed on the implementations of the persistence providers that are called from the service methods (though if they are DB backed most DB engines will take care of that for you). But I agree, as implemented here, it's too much of a performance trade off if we want to support high concurrency / number of clients for a C2 server instance.
I'll take another pass at this and look at it on more of a case-by-case basis.
---
[GitHub] nifi-minifi pull request #120: MINIFI-448 C2 server functionality and intern...
Posted by kevdoran <gi...@git.apache.org>.
Github user kevdoran commented on a diff in the pull request:
https://github.com/apache/nifi-minifi/pull/120#discussion_r177824172
--- Diff: minifi-c2/minifi-c2-framework/src/main/java/org/apache/nifi/minifi/c2/api/provider/device/DevicePersistenceProvider.java ---
@@ -0,0 +1,78 @@
+/*
+ * 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.minifi.c2.api.provider.device;
+
+import org.apache.nifi.minifi.c2.api.provider.Provider;
+import org.apache.nifi.minifi.c2.model.Device;
+
+import java.util.List;
+import java.util.Optional;
+
+/**
+ * NOTE: Although this interface is intended to be an extension point, it is not yet considered stable and thus may
+ * change across releases until the the C2 Server APIs mature.
+ *
+ * TODO, we may want to consider creating a separate entity model rather than reusing the REST API object model.
+ * Currently, this design assumes the Provider implementation will do that translation.
+ * This requires adding a dependency on minifi-c2-commons here for the data model.
+ */
+public interface DevicePersistenceProvider extends Provider {
--- End diff --
> Would it make sense to have a base interface kind of like the Spring Data repositories
I think this makes a lot of sense. I'm going to create this base interface type as you suggested. Are there any other methods you can think of that should be included the base interface? (It has everything needed for the current framework invocations, but expecting third-parties might provide an implementation, is there anything else we can anticipate wanting provided?)
> So the only pluggable provider is the top-level AgentPersistenceProvider, but then it is up to that provider to provider implementations of the sub-providers.
Yeah I like this approach. Going to do that for now and we can refine it (if needed) as these provider interfaces mature a bit.
---
[GitHub] nifi-minifi pull request #120: MINIFI-448 C2 server functionality and intern...
Posted by jzonthemtn <gi...@git.apache.org>.
Github user jzonthemtn commented on a diff in the pull request:
https://github.com/apache/nifi-minifi/pull/120#discussion_r177855956
--- Diff: minifi-c2/minifi-c2-framework/src/test/groovy/org/apache/nifi/minifi/c2/core/service/StandardC2ProtocolServiceSpec.groovy ---
@@ -0,0 +1,164 @@
+/*
+ * 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.minifi.c2.core.service
+
+import org.apache.nifi.minifi.c2.api.provider.heartbeat.HeartbeatPersistenceProvider
+import org.apache.nifi.minifi.c2.model.*
+import spock.lang.Specification
+
+class StandardC2ProtocolServiceSpec extends Specification {
--- End diff --
Thanks for the details! Sounds like something worth checking out. I remember seeing a discussion not too long ago on the mailing list concerning JVM-based languages in the code base. (I don't remember if anything actually came out of it or not.) Anyway, I think it's pretty cool because otherwise I probably wouldn't have been exposed to this.
---