MINIFICPP-740: Add ability to run NiFi processors from Java and Python #489

phrocker · 2019-02-21T16:44:00Z

Thank you for submitting a contribution to Apache NiFi - MiNiFi C++.

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

If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?
If applicable, have you updated the LICENSE file?
If applicable, have you updated the NOTICE file?

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.

phrocker · 2019-02-21T16:45:20Z

not satisfied that I put all java code underneath one package. that was pure laziness. I will move those out at some point ( either in a follow on ticket or in this one )

CMakeLists.txt

apiri · 2019-02-22T02:26:21Z

reviewing

phrocker · 2019-02-22T02:27:59Z

reviewing

good luck

apiri · 2019-02-22T04:18:58Z

This is going to be the best functionality we add in 2020!

arpadboda

Added some comments, but nowhere near the end yet, this is A LOT.

arpadboda · 2019-02-25T15:56:51Z

extensions/expression-language/Expression.cpp

+      if ( registry && registry->getConfigurationProperty( attribute_id , result) ) {
+        return Value(result);
+      }
+      else {


This branch is needless. (Maybe it was my bad in my PR)

arpadboda · 2019-02-25T16:04:24Z

extensions/jni/ExecuteJavaProcessor.cpp

+
+  jni_logger_ref_.logger_reference_ = nifi_logger_;
+
+  std::string dir_name_;


This seems to be unreferenced. (Guess it's just left)

arpadboda · 2019-02-25T16:13:21Z

extensions/jni/ExecuteJavaProcessor.cpp

+  init = java_servicer_->loadClass("org/apache/nifi/processor/JniInitializationContext");
+
+  if (context_instance_ != nullptr) {
+    java_servicer_->attach()->DeleteGlobalRef(context_instance_);


Can't somehow JniProcessContext be responsible for the ref handling?
It would be nice to have it in RAII.

It cannot. The lifetime of the object is inherently tied to a context, session, and flow file within Java.

Oh and to give more context: ProcessContext is probably the only object where we might be able to use RAII. The other objects ( flow file, session, and session factory we cannot ). As a result I made the painful decision to return the raw jobject versus a wrapped object. There are some things I'd like to improve in this class but hope to do so in subsequent releases.

arpadboda · 2019-02-25T16:17:56Z

extensions/jni/ExecuteJavaProcessor.h

+    if (registered_classes_.find(className) == std::end(registered_classes_)) {
+      auto cls = servicer->loadClass(className);
+      cls.registerMethods(env, signatures);
+      registered_classes_.insert(className);


std::set::insert supports exactly what you need:

1-2) Returns a pair consisting of an iterator to the inserted element (or to the element that prevented the insertion) and a bool value set to true if the insertion took place.

So you can avoid using find.

added a comment as to why find was chosen

arpadboda · 2019-02-25T16:28:05Z

extensions/jni/ExecuteJavaProcessor.h

+
+    JniSessionFactory *factory = new JniSessionFactory(ptr, java_servicer_, obj);
+
+    session_factories_.push_back(factory);


Guess the mutex should be locked here, too. (Vector::push_back may result in moving elements)

This is something I'm still testing. may not need more than one, in which case I Just applied a setter -- but will lock if this persists.

I added the lock in the event I forget to remove this code -- but it seems like I can get away with just one ( hence why it was a setter versus being called add ). Please don't resolve this convo so it's a placesetter so I can respond in the even the code gets removed. Thanks.

arpadboda · 2019-02-25T16:31:48Z

extensions/jni/JNILoader.h

+   * @return class name for the processor.
+   */
+  virtual std::vector<std::string> getClassNames() {
+    std::vector<std::string> class_names;


Sure, that won't have any negatives at all and certainly will help if someone were to call it many times thanks.

arpadboda · 2019-02-25T16:34:32Z

extensions/jni/JVMCreator.cpp

+        }
+        if (S_ISDIR(statbuf.st_mode)) {
+          // if this is a directory
+          if (strcmp(d_name.c_str(), "..") != 0 && strcmp(d_name.c_str(), ".") != 0) {


What's the reason of using strcmp here?

That's a good point. I copied this from elsewhere in the code as it did something similar, but could have easily used stringutils for that.

arpadboda · 2019-02-25T16:46:50Z

extensions/jni/jvm/JVMLoader.h

+    }
+
+    std::string modifiedName = name;
+    modifiedName = utils::StringUtils::replaceAll(modifiedName, "/", ".");


This is exactly what we expect clazz_name to be, isn't it?

Not necessarily ( and not always ) but I will add a comment here. Thanks

arpadboda · 2019-02-25T16:54:57Z

extensions/jni/jvm/JVMLoader.h

+      std::vector<std::string> options;
+      for (const auto &path : pathVector) {
+        if (str.str().length() > 0) {
+#ifdef WIN32


#ifdef WIN32 const char * delimiter = ";"; #else const char * delimiter = ":"; #endif stringstream str; std::copy(pathVector.begin(), pathVector.end(), std::ostream_iterator<std::string>(str, delimiter));

A bit Pythonic, but I guess easier to read.

Yep, just trying to limit trailing characters.

Does pathVector contain empty strs?

MINIFICPP-740: Fix autoconf location issue MINIFICPP-740: minor updates MINIFICPP-740: Topologically sort dependencies MINIFICPP-740: Updates to package and jni subproject

arpadboda · 2019-02-26T09:51:55Z

extensions/jni/jvm/JavaControllerService.cpp

+        }
+        if (S_ISDIR(statbuf.st_mode)) {
+          // if this is a directory
+          if (strcmp(d_name.c_str(), "..") != 0 && strcmp(d_name.c_str(), ".") != 0) {


This seems to be nearly the same as "JVMCreator::addPath".

Can we make it an util function to avoid duplication?

Yep! Thanks!

arpadboda · 2019-02-26T10:01:26Z

extensions/jni/jvm/JniFlowFile.cpp

+
+  auto ff = ptr->get();
+  if (ff == nullptr) {
+    minifi::jni::ThrowJava(env, "Calling function on null flow file");


I think using a util func or just a macro, like

checkFlowFile(env, ff);

would be useful to avoid duplicating the same error message.

I'm going through and doing cleanup now... and I extracted a lot of those string references to macros -- but I just saw this in an E-mail ( haven't gone through the rest ) -- just wanted to comment that I really like this idea. Thanks!

arpadboda · 2019-02-26T10:03:11Z

extensions/jni/jvm/JniLogger.cpp

+  return true;
+}
+jboolean Java_org_apache_nifi_processor_JniLogger_isInfoEnabled(JNIEnv *env, jobject obj) {
+  minifi::jni::JniLogger *logger_ref = minifi::jni::JVMLoader::getPtr<minifi::jni::JniLogger>(env, obj);


Could you explain what's happening here?
I would expect the to check the logger (to see if [info, error, trace] is enabled) and return that.

Sorry I did some cleanup after I submitted review. I didn't realize you submitted this. I think these are addressed.

arpadboda · 2019-02-26T10:15:05Z

extensions/jni/jvm/JniLogger.cpp

+  return true;
+}
+
+void Java_org_apache_nifi_processor_JniLogger_warn(JNIEnv *env, jobject obj, jstring msg) {


I think a private function with a log level parameter could help here:

void Java_org_apache_nifi_processor_JniLogger_log(JNIEnv *env, jobject obj, jstring msg, spdlog::level log_level) { minifi::jni::JniLogger *logger_ref = minifi::jni::JVMLoader::getPtr<minifi::jni::JniLogger>(env, obj); if (!logger_ref) return; const char *msgStr = env->GetStringUTFChars(msg, 0); logger_ref->logger_reference_->log(log_level, msgStr); env->ReleaseStringUTFChars(msg, msgStr); }

So the rest can be done in one line, like:

void Java_org_apache_nifi_processor_JniLogger_warn(JNIEnv *env, jobject obj, jstring msg) { ava_org_apache_nifi_processor_JniLogger_log(env, ojb, msg, spdlog::level::warn) }

Thanks for your suggestion.

arpadboda · 2019-02-26T10:29:48Z

extensions/jni/jvm/JniMethod.h

+      method_ptr_ = std::unique_ptr<JNINativeMethod>(new JNINativeMethod[methods_.size()]);
+      size_ = methods_.size();
+      int i = 0;
+      for (auto &mthd : methods_) {


I think it would make more sense to just increment "i" in the for loop and use that to index methods_ as well, the mixture of the two (range-based for and using indexes) is a bit fuzzy in my opinion.

ha I don't know why I did that. Thanks for finding it!

arpadboda · 2019-02-26T12:29:20Z

extensions/jni/jvm/JniReferenceObjects.h

+
+  virtual void remove() override;
+
+  std::shared_ptr<core::FlowFile> get() {


I made some changes before seeing some of these. I think many of these were caught. But I may have missed something.

arpadboda · 2019-02-26T12:29:41Z

extensions/jni/jvm/JniReferenceObjects.h

+    return ff_object;
+  }
+
+  bool operator==(const JniFlowFile &other) {


Ugh...Eclipse doesn't auto add the const when it generates these functions. Thanks!

arpadboda · 2019-02-26T12:29:52Z

extensions/jni/jvm/JniReferenceObjects.h

+    return ref_ == other.ref_;
+  }
+
+  bool empty() {


I made some changes before seeing some of these. I think many of these were caught, including this one

arpadboda · 2019-02-26T12:39:10Z

extensions/jni/jvm/JniReferenceObjects.h

+
+ protected:
+  bool removed_;
+  std::mutex session_mutex_;


Can we make this mutable and make some member funcs const?

I made some changes before seeing some of these. I think many of these were caught, including this one

arpadboda · 2019-02-26T12:46:11Z

extensions/jni/jvm/NarClassLoader.h

+    }
+    {
+
+      jmethodID mthd = env->GetMethodID(class_ref_.getReference(), "getSignature", "(Ljava/lang/String;Ljava/lang/String;)Ljava/lang/String;");


These blocks have so much in common, can it be a loop?

Most likely. I'll catch it on another round of cleanup. thanks!

arpadboda · 2019-02-26T13:40:41Z

extensions/script/README.md

+	  processor.setSupportsDynamicProperties()
+	  processor.addProperty("property name","description","default value", True, False)
+```
+


I made a previous comment here, which I don't see now, so sorry for the duplication in case that happens!

Things I saw in the code and worth documenting in my opinion:
-getProperty call supports getting value of dynamic properties, too. (as it's different in C++, I think it would be beneficial to have it documented)
-Python processors support success and failure relationships, these are the ones the script can send to.

A question: what happens in case there is a Python exception thrown in "onTrigger"?
I wonder if a rollback should be triggered.

It will yield, as execute script does, if the python writer doesn't actually do the legwork of rolling back.

arpadboda · 2019-02-26T13:46:23Z

extensions/script/python/PythonCreator.cpp

+        }
+        if (S_ISDIR(statbuf.st_mode)) {
+          // if this is a directory
+          if (strcmp(d_name.c_str(), "..") != 0 && strcmp(d_name.c_str(), ".") != 0) {


The same strcmp usage here as in Java

arpadboda · 2019-02-26T13:49:10Z

libminifi/include/FlowController.h

@@ -339,6 +339,8 @@ class FlowController : public core::controller::ControllerServiceProvider, publi
  // function to load the flow file repo.
  void loadFlowRepo();

+  void initializeJNI();


What happens if JNI extension is not built/added?

If a referenced processor isn't there we'll error out and tell them. Otherwise this will be a non issue.

arpadboda · 2019-02-26T13:51:21Z

libminifi/include/agent/build_description.h

+    } else {
+      getExternalMappings()[details.artifact].processors_.push_back(description);
+    }
+    return true;


The function always returns true, does it make sense to have bool return type?

arpadboda · 2019-02-26T13:55:16Z

libminifi/include/core/VariableRegistry.h

+        options.erase(std::remove(options.begin(), options.end(), opt), options.end());
+    }
+
+    // even if a white list is configured, remove the black listed fields


What's the reason behind having both whitelist and blacklist?

The comment does this a disservice but white listed fields allow selection only, blacklisted fields allow removal of fields from registry. 2 options, either it's an error condition to have both or we just remove blacklisted fields i the white list is specified. that was my preference for COOP.

arpadboda · 2019-02-26T14:01:15Z

libminifi/include/utils/file/FileManager.h

    while (!verify_not_exist(file_name)) {
-      file_name = "/tmp/" + non_repeating_string_generator_.generate();
+      file_name = tmpDir + FILE_SEPARATOR + non_repeating_string_generator_.generate();


Filesystem in cpp17 is really missed here... :(

arpadboda · 2019-02-26T14:13:25Z

thirdparty/pybind11/include/pybind11/pytypes.h

@@ -292,7 +292,7 @@ class error_already_set : public std::runtime_error {
    /// Constructs a new exception from the current Python error indicator, if any.  The current
    /// Python error indicator will be cleared.
    error_already_set() : std::runtime_error(detail::error_string()) {
-        PyErr_Fetch(&type.ptr(), &value.ptr(), &trace.ptr());
+        //PyErr_Fetch(&type.ptr(), &value.ptr(), &trace.ptr());


Don't we need this?

I created a follow on ticket to explore this further -- but this was causing problems. I read several old tickets where people removed this line in regards to triggering seg faults. Usually this is an issue elsewhere, but ExecuteScript was causing problems. In most cases it was fine because we weren't really handling python exceptions, but I created a follow on because at some point I would like to better deal with that.

minor updates

arpadboda · 2019-02-26T23:02:28Z

Looks good as far as I can tell! The comments I considered important are resolved.

@phrocker : could you make an exemption and add cleanups or anything else added later as a separate commit instead of amend?
The PR is very big, so it's difficult to find the changes. Thanks in advance!

apiri

had some minor comments on things but the functionality works well. I performed builds/testing on MacOS, Ubuntu 18.04, and CentOS 7 using both NARs and processors defined in python. Everything worked fairly well with the Ubuntu and CentOS items running for the past several days.

Of my comments, I think we need to tend to the licensing concerns before merge, but everything else can be addressed later if needed. I saw a few comments that seemed to still be open from @arpadboda. If you could give those a scan through and make sure you're good with those, I can take care of licensing and get this merged in.

apiri · 2019-02-22T16:09:37Z

extensions/jni/nifi-framework-jni/nifi-framework-jni.iml

@@ -0,0 +1,127 @@
+<?xml version="1.0" encoding="UTF-8"?>


looks like we need gitignore for java in our c++ 😱

apiri · 2019-02-22T16:14:44Z

CMakeLists.txt

+	if (ENABLE_JNI)
+	  set(BASE_DIR "${CMAKE_CURRENT_BINARY_DIR}/thirdparty/jemalloc")
+
+	  set(DIR "${BASE_DIR}/extensions/jemalloc-src")


Will need to capture this in our LICENSE (2-clause bsd)

apiri · 2019-02-22T16:18:20Z

extensions/script/README.md

+followed by the description, and default value. The last two arguments are booleans describing if the property is required or requires EL.
+
+```python
+	def onIitialize(processor):


onInitialize

apiri · 2019-02-22T16:21:31Z

extensions/script/README.md

+
+Methods that are enabled within the processor are  describe, onSchedule, onInitialize, and onTrigger. 
+
+Describe is passed the a processor and is a required function. You must set the description like so:


is passed the processor

apiri · 2019-02-22T16:24:24Z

extensions/script/README.md

+```
+
+
+   void addProperty(const std::string &name, const std::string &description, const std::string &defaultvalue, bool required, bool el) {


a little confused as to why this is here. did you mean to include it as one of the available methods within the processor? There is also a mention of getProperty below.

apiri · 2019-02-22T16:53:36Z

extensions/jni/nifi-framework-jni/pom.xml

+  See the License for the specific language governing permissions and
+  limitations under the License.
+-->
+<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/maven-v4_0_0.xsd">


Need to have NiFi in our L&N

apiri · 2019-02-22T17:00:47Z

extensions/jni/nifi-framework-jni/src/main/java/org/apache/nifi/nar/JniUnpacker.java

+
+            final File bundledDependencies = new File(unpackedNar, "NAR-INF/bundled-dependencies");
+
+            unpackBundleDocs(docsDirectory, mapping, bundleCoordinate, bundledDependencies);


Don't think we need to do this as the docs will not be used/exposed in the minifi context

The documentation provides useful info in the jar entries ( meta-inf/services/*) that could help us supplement the service building the agent manifest info

nm i don't think i could really get all the information I need without loading processors anyway. good point re the docs.

apiri · 2019-02-22T17:01:43Z

extensions/jni/nifi-framework-jni/src/main/java/org/apache/nifi/nar/JniUnpacker.java

@@ -0,0 +1,398 @@
+/*


This and other Java that is substantially based off of NiFi should be treated as a source inclusion in our L&N. This is mostly just enumerating the particular files we are including given the ALv2 license.

oh man missed this. thanks

apiri · 2019-02-22T17:02:26Z

extensions/jni/nifi-framework-jni/src/main/java/org/apache/nifi/nar/JniUnpacker.java

+
+            // attempt to delete any docs files that exist so that any components that have been removed
+            // will no longer have entries in the docs folder
+            final File[] docsFiles = docsWorkingDir.listFiles();


doc work should be irrelevant

arpadboda · 2019-02-28T10:36:58Z

@phrocker:
I don't like the fact that JNI API doesn't provide RAII wrapper from UTF string handling, so I quickly scratched one:

class JNIUTFString {
 public:
  JNIUTFString(JNIEnv * env, jstring jstr) {
    _env = env;
    _jstr = jstr;
    _cstr = _env->GetStringUTFChars(_jstr, NULL);
  }
  ~JNIUTFString() {
    _env->ReleaseStringUTFChars(_jstr, _cstr);
  }
  
  operator std::string() const { return _cstr; }
  
  const char * c_str() const { return _cstr; }
  
  friend std::ostream& operator<<(std::ostream& os, const JNIUTFString& jutfstr);
  
 private:
  JNIEnv * _env;
  const char * _cstr;
  jstring _jstr;
};

std::ostream& operator<<(std::ostream& os, const JNIUTFString& jutfstr)
{
    os << jutfstr.c_str();
    return os;
}

This can be used as an std::string, but you don't have to deal with conversion, releasing, etc, usage is simple:

  JNIUTFString jutf(env, jstr);
  std::set<std::string> test_set;
  test_set.insert(jutf);
  std::cout << jutf <<std::endl;
  std::cout << jutf.c_str() << std::endl;

Feel free to add if you like it.

phrocker · 2019-02-28T17:39:59Z

@apiri I didn't resolve convos because I believe that is up to the author of that comment; however, I believe I addressed most issues. please take another gander. I'm not sure if that last comment was directed at me or Arpad, though.

apiri · 2019-02-28T17:41:18Z

@phrocker sounds fair. will do one last scan/build/test a bit later and we can move on. thanks!

This closes apache#489. Signed-off-by: Aldrin Piri <aldrin@apache.org>

phrocker requested a review from apiri February 21, 2019 16:44

phrocker commented Feb 21, 2019

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

phrocker force-pushed the jnimer branch 2 times, most recently from dc6ef70 to 4447798 Compare February 22, 2019 15:02

phrocker mentioned this pull request Feb 23, 2019

NIFI-6072: Add sub project to package nifi-minifi-jni required NARs a… apache/nifi#3330

Closed

11 tasks

phrocker force-pushed the jnimer branch from 09ab81e to 399427b Compare February 25, 2019 03:17

arpadboda reviewed Feb 25, 2019

View reviewed changes

phrocker force-pushed the jnimer branch from 694bcee to 7c8e5a5 Compare February 25, 2019 20:22

arpadboda and others added 2 commits February 25, 2019 15:23

MINIFICPP-738 - EL should be able to access global properties

4fb8665

MINIFICPP-740: Add ability to run NiFi processors from Java and Python

826dfcc

MINIFICPP-740: Fix autoconf location issue MINIFICPP-740: minor updates MINIFICPP-740: Topologically sort dependencies MINIFICPP-740: Updates to package and jni subproject

phrocker force-pushed the jnimer branch from 7c8e5a5 to 778d451 Compare February 25, 2019 20:30

arpadboda reviewed Feb 26, 2019

View reviewed changes

MINIFICPP-740: resolve appveyor error

7a732a4

minor updates

phrocker force-pushed the jnimer branch from 4fba82a to 7a732a4 Compare February 26, 2019 18:40

MINIFICPP-740: Fix missing dep on windows

e01e109

phrocker force-pushed the jnimer branch from 42c2c17 to e01e109 Compare February 26, 2019 20:38

phrocker closed this Feb 26, 2019

phrocker reopened this Feb 26, 2019

This comment has been minimized.

Sign in to view

apiri approved these changes Feb 27, 2019

View reviewed changes

phrocker added 2 commits February 27, 2019 20:58

MINIFICPP-740: updates per comments

a697c76

MINIFICPP-740: remove docs dir from readme

71c60fd

phrocker force-pushed the jnimer branch from 0b51a09 to 71c60fd Compare February 28, 2019 03:05

MINIFICPP-740: review comment

94bd1b4

phrocker mentioned this pull request Feb 28, 2019

POC: MINIFICPP-738 - EL should be able to access global properties #487

Closed

8 tasks

asfgit closed this in 8a4c2ac Mar 1, 2019

nghiaxlee pushed a commit to nghiaxlee/nifi-minifi-cpp that referenced this pull request Jul 8, 2019

MINIFICPP-740: Add ability to run NiFi processors from Java and Python

5759ddb

This closes apache#489. Signed-off-by: Aldrin Piri <aldrin@apache.org>


		jni_logger_ref_.logger_reference_ = nifi_logger_;

		std::string dir_name_;


		JniSessionFactory *factory = new JniSessionFactory(ptr, java_servicer_, obj);

		session_factories_.push_back(factory);


		virtual void remove() override;

		std::shared_ptr<core::FlowFile> get() {


		Methods that are enabled within the processor are describe, onSchedule, onInitialize, and onTrigger.

		Describe is passed the a processor and is a required function. You must set the description like so:

		```


		void addProperty(const std::string &name, const std::string &description, const std::string &defaultvalue, bool required, bool el) {


		final File bundledDependencies = new File(unpackedNar, "NAR-INF/bundled-dependencies");

		unpackBundleDocs(docsDirectory, mapping, bundleCoordinate, bundledDependencies);

MINIFICPP-740: Add ability to run NiFi processors from Java and Python #489

MINIFICPP-740: Add ability to run NiFi processors from Java and Python #489

Conversation

phrocker commented Feb 21, 2019

For all changes:

For code changes:

For documentation related changes:

Note:

phrocker commented Feb 21, 2019

apiri commented Feb 22, 2019

phrocker commented Feb 22, 2019

apiri commented Feb 22, 2019

arpadboda left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phrocker Feb 25, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phrocker Feb 26, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arpadboda commented Feb 26, 2019 • edited

This comment has been minimized.

apiri left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phrocker Feb 28, 2019 • edited

phrocker Feb 25, 2019 •

edited

phrocker Feb 26, 2019 •

edited

arpadboda commented Feb 26, 2019 •

edited

phrocker Feb 28, 2019 •

edited