[GitHub] [unomi] jbonofre commented on a change in pull request #156: Enable Kafka Buffer via RuleEngine(Action)

[GitBox <gi...@apache.org>]

jbonofre commented on a change in pull request #156:
URL: https://github.com/apache/unomi/pull/156#discussion_r522620464



##########
File path: actions/pom.xml
##########
@@ -0,0 +1,15 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<project xmlns="http://maven.apache.org/POM/4.0.0"
+         xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+         xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
+    <parent>

Review comment:
       ASF header is missing there.

##########
File path: extensions/data-operation/data-operation-actions/pom.xml
##########
@@ -0,0 +1,219 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<project xmlns="http://maven.apache.org/POM/4.0.0"
+         xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+         xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
+    <parent>

Review comment:
       Please add ASF header.

##########
File path: extensions/data-operation/data-operation-karaf-kar/src/main/feature/feature.xml
##########
@@ -0,0 +1,26 @@
+<?xml version="1.0" encoding="UTF-8" standalone="yes"?>
+<!--
+  ~ 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.
+  -->
+<features xmlns="http://karaf.apache.org/xmlns/features/v1.2.1" name="data-operation-karaf-feature">
+    <feature name="data-operation-karaf-kar" version="${project.version}"
+             description="MetaRouter :: Extensions :: Data Operation">
+        <details>Connect Apache Unomi to PrimeData</details>
+        <bundle start-level="90">mvn:org.apache.unomi/data-operation-actions/${project.version}</bundle>

Review comment:
       Why not using default start level ?

##########
File path: extensions/data-operation/data-operation-actions/src/main/java/org/apache/unomi/operation/router/EventProducer.java
##########
@@ -0,0 +1,8 @@
+package org.apache.unomi.operation.router;

Review comment:
       Please add ASF header.

##########
File path: docker/src/main/docker/docker-compose.yml
##########
@@ -28,13 +28,16 @@ services:
       - cluster.name=contextElasticSearch
     ports: # Expose Elasticsearch ports
       - "9200:9200"
-
+    networks:
+      - unomi
   unomi:
     build: .
     image: apache/unomi:${project.version}
     container_name: unomi
     environment:
       - UNOMI_ELASTICSEARCH_ADDRESSES=elasticsearch:9200
+      - KAFKA_BROKERS=kafka:9093

Review comment:
       It should be still optional. I would rather add another docker-compose with Kafka (lot of users don't care about Kafka for now).

##########
File path: extensions/data-operation/data-operation-actions/README.md
##########
@@ -0,0 +1,41 @@
+#Enable kafka buffer

Review comment:
       Please add ASF header.

##########
File path: extensions/data-operation/data-operation-actions/src/main/java/org/apache/unomi/operation/actions/BufferEventProcessingAction.java
##########
@@ -0,0 +1,25 @@
+package org.apache.unomi.operation.actions;

Review comment:
       Please add ASF header.

##########
File path: package/src/main/resources/etc/custom.properties
##########
@@ -29,3 +29,11 @@ karaf.systemBundlesStartLevel=50
 #
 
 org.osgi.framework.bootdelegation=org.apache.karaf.jaas.boot,org.apache.karaf.jaas.boot.principal,sun.*,com.sun.*,javax.transaction,javax.transaction.*,javax.xml.crypto,javax.xml.crypto.*,org.apache.xerces.jaxp.datatype,org.apache.xerces.stax,org.apache.xerces.parsers,org.apache.xerces.jaxp,org.apache.xerces.jaxp.validation,org.apache.xerces.dom,javax.management.remote.rmi,com.yourkit.*
+
+#
+# Kafka Properties reflects Apache Camel
+#
+org.apache.operation.router.kafka.consumersCount=10

Review comment:
       The properties don't look good to me (it should be at least org.apache.unomi). Furthermore I think it's better to use a PID instead of system property.

##########
File path: extensions/data-operation/data-operation-actions/src/main/java/org/apache/unomi/operation/router/EventKafkaContextProducer.java
##########
@@ -0,0 +1,105 @@
+package org.apache.unomi.operation.router;
+
+import org.apache.camel.ProducerTemplate;
+import org.apache.camel.builder.RouteBuilder;
+import org.apache.camel.component.jackson.JacksonDataFormat;
+import org.apache.camel.component.kafka.KafkaComponent;
+import org.apache.camel.component.kafka.KafkaConfiguration;
+import org.apache.camel.component.kafka.KafkaEndpoint;
+import org.apache.camel.core.osgi.OsgiDefaultCamelContext;
+import org.apache.unomi.api.Event;
+import org.osgi.framework.BundleContext;
+import org.osgi.framework.BundleEvent;
+import org.osgi.framework.SynchronousBundleListener;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.util.Map;
+
+public class EventKafkaContextProducer implements SynchronousBundleListener, EventProducer {
+    private OsgiDefaultCamelContext camelContext;
+    private BundleContext bundleContext;
+    protected String kafkaTopic = "eventify-event";
+    private ProducerTemplate producer;
+    private JacksonDataFormat objectMapper;
+    private static Logger logger = LoggerFactory.getLogger(EventKafkaContextProducer.class);
+
+    private Map<String, String> kafkaProps;
+
+    public void initCamelContext() throws Exception {

Review comment:
       I don't think we need Camel here, regarding the route, it could be implemented directly.
   
   It introduces lot of dependencies for not lot of gain.
   
   More other, the camelcontext should be registered as service (to be visible by camel:* commands).

##########
File path: extensions/data-operation/data-operation-actions/src/main/java/org/apache/unomi/operation/router/EventKafkaContextProducer.java
##########
@@ -0,0 +1,105 @@
+package org.apache.unomi.operation.router;

Review comment:
       Please add ASF header.




----------------------------------------------------------------
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