WIP: Persistent logging Implementation #7

cmbuhler · 2020-03-13T21:21:04Z

No description provided.

mpirvu

I have a few inlined comments on the code.
General observations:

Try to follow our coding conventions regarding placement of curly brackets and indentation
Avoid the use of iostream. Use printf/fprintf instead
If possible, use C strings instead of std::string. If that is not possible we need to find a way to allocate backing memory for those std::string with our region allocators which is freed automatically at the end of a compilation. In general be mindful of where memory is allocated and free it as soon as possible.

mpirvu · 2020-03-14T02:13:26Z

runtime/compiler/control/BasePersistentLogger.hpp

+
+class BasePersistentLogger {
+protected:
+    std::string _databaseIP;


These could be marked as const because they are set in the constructor and never changed afterwards

mpirvu · 2020-03-14T02:18:22Z

runtime/compiler/control/CassandraLogger.hpp

+
+
+    public: 
+        bool connect();


connect, disconnect and logMethod should be marked virtual and override. Not required, but we prefer it that way.

mpirvu · 2020-03-14T02:19:20Z

runtime/compiler/control/CassandraLogger.hpp

+
+#include <cassandra.h>
+#include "BasePersistentLogger.hpp"
+class CassandraLogger : public BasePersistentLogger {


Please follow our coding conventions and place the curly bracket on the next line, indented by 3 spaces.

mpirvu · 2020-03-14T02:20:34Z

runtime/compiler/control/CassandraLogger.cpp

@@ -0,0 +1,200 @@
+#include <time.h>
+#include <iostream>


Avoid using iostream and use printf style if at all possible.

mpirvu · 2020-03-14T02:21:45Z

runtime/compiler/control/CassandraLogger.cpp

+#include <iostream>
+
+#include "CassandraLogger.hpp"
+CassandraLogger::CassandraLogger(std::string const &databaseIP, std::uint32_t databasePort, std::string const &databaseName): BasePersistentLogger(databaseIP, databasePort, databaseName){


Please follow our coding conventions regarding the curly braces

mpirvu · 2020-03-14T03:01:20Z

runtime/compiler/control/rossa.cpp

      }
+#if defined(PERSISTENT_LOGGING_SUPPORT)
+//TODO: FIx this flag
+   //if(TR::Options::getCmdLineOptions()->getOption(TR_PersistLogging))


This option should be used at the client in my opinion. The server should be controlled by those -XX: options. Do we need another option?

mpirvu · 2020-03-14T03:04:39Z

runtime/compiler/control/MongoLogger.hpp

+
+class MongoLogger : public BasePersistentLogger {
+private:
+    Omongoc_uri_t *_uri;


These pointers need to be initialized in the constructors.

mpirvu · 2020-03-14T03:05:30Z

runtime/compiler/control/MongoLogger.cpp

+
+MongoLogger::~MongoLogger() {
+    //Clean up this mongoc logger.
+    Omongoc_collection_destroy(_collection);


Is it safe to destroy NULL pointers?

Yes it is. The mongoc functions will do nothing if they receive a NULL pointer.

mpirvu · 2020-03-14T03:10:33Z

runtime/compiler/control/JITServerCompilationThread.cpp

+      std::cout << "what is the persistent logging database Name ? " << persistentLoggingDatabaseName << std::endl;
+
+#ifdef CASSANDRA_LOGGER
+      CassandraLogger* logger = new CassandraLogger(persistentLoggingDatabaseAddress, std::persistentLoggingDatabasePort, persistentLoggingDatabaseName,persistentLoggingDatabaseUsername, persistentLoggingDatabasePassword);


A new logger is created dynamically, but who destroys it? If you plan on not reusing it, then allocate it on stack rather than with new.

mpirvu · 2020-03-14T03:10:40Z

runtime/compiler/control/JITServerCompilationThread.cpp

+#endif // CASSANDRA_LOGGER
+
+#ifdef MONGO_LOGGER
+      MongoLogger* logger = new MongoLogger(persistentLoggingDatabaseAddress, persistentLoggingDatabasePort, persistentLoggingDatabaseName,persistentLoggingDatabaseUsername, persistentLoggingDatabasePassword);


A new logger is created dynamically, but who destroys it? If you plan on not reusing it, then allocate it on stack rather than with new.

mpirvu · 2020-03-26T20:53:02Z

runtime/compiler/control/BasePersistentLogger.hpp

This assumes that the strings receives as parameters continue to exist after the object is constructed.
I'll have to verify if that's the case.

mpirvu · 2020-03-26T20:53:29Z

runtime/compiler/control/BasePersistentLogger.hpp

Extra space after first (

mpirvu · 2020-03-26T21:14:35Z

runtime/compiler/control/J9Options.cpp

Let's move this below to the next section protected by #ifdef PERSISTENT_LOGGING_SUPPORT

mpirvu · 2020-03-26T21:15:25Z

runtime/compiler/control/JITServerCompilationThread.cpp

No iostream please

mpirvu · 2020-03-26T21:22:48Z

runtime/compiler/control/JITServerCompilationThread.cpp

Extra spaces here

mpirvu · 2020-03-26T21:49:28Z

runtime/compiler/control/MongoLogger.cpp

Some alignment issues

mpirvu · 2020-03-26T22:10:49Z

runtime/compiler/control/MongoLogger.hpp

If it's statically allocated you don't need allocators.The space is big though, so there is a danger of stack overflow. Maybe reducing that to 256 is sufficient.

mpirvu · 2020-03-26T22:17:01Z

runtime/compiler/control/rossa.cpp

This is executed when jitserver boostraps. We need a master option that enables persistent logging at runtime. Getting the option from the client will not do.

mpirvu · 2020-03-26T22:22:12Z

runtime/compiler/control/MongoLogger.cpp

maybe we can use some time routines from omr. Do you need the time in a specific format?

mpirvu · 2020-03-26T22:24:09Z

runtime/compiler/control/CassandraLogger.cpp

Do we need this buffer that big? Only database name is variable, so I am guessing that 256 chars is enough.
Also use snprintf to avoid buffer overflow.

fixed namespace error retrieved method signiture a new way of retrieving method full name changed to an easier way to retrieve compiled method signature attempted to add -XX:persistentLoggingDatabasePort flag, hit segmentation fault.. added if clause for persistent logging, retrieved clientUID info fixed namespace error retrieved method signiture Attempt to add logger to openj9. make Changes and stuff Add conditionals to check for mongo and cassandra support -XX flags needed for persistent logging completed Fix an issue with my previous merge IMplement environment variable ifdef wrapper thing port initialization on different DBMS added if clause for persistent logging, retrieved clientUID info a new way of retrieving method full name changed to an easier way to retrieve compiled method signature attempted to add -XX:persistentLoggingDatabasePort flag, hit segmentation fault.. retrieved method signiture Attempt to add logger to openj9. Add conditionals to check for mongo and cassandra support -XX flags needed for persistent logging completed IMplement environment variable ifdef wrapper thing add general persistent logging support flag for checking if persistent logging is enabled regardless of dbms fix more merge conflicts I forgot Add more env variable processing Add fix that allows you to enable the cassandra logger debug TR_PersistLogging flag Bring in changes by Ida for persistentLogger spec, dynamically load mongoc library addressed code review feedback addressed code review feedback addressed code review feedback added if clause for persistent logging, retrieved clientUID info fixed namespace error retrieved method signiture a new way of retrieving method full name changed to an easier way to retrieve compiled method signature attempted to add -XX:persistentLoggingDatabasePort flag, hit segmentation fault.. added if clause for persistent logging, retrieved clientUID info fixed namespace error retrieved method signiture Attempt to add logger to openj9. make Changes and stuff Add conditionals to check for mongo and cassandra support -XX flags needed for persistent logging completed Fix an issue with my previous merge IMplement environment variable ifdef wrapper thing port initialization on different DBMS added if clause for persistent logging, retrieved clientUID info a new way of retrieving method full name changed to an easier way to retrieve compiled method signature attempted to add -XX:persistentLoggingDatabasePort flag, hit segmentation fault.. retrieved method signiture Attempt to add logger to openj9. Add conditionals to check for mongo and cassandra support -XX flags needed for persistent logging completed IMplement environment variable ifdef wrapper thing add general persistent logging support flag for checking if persistent logging is enabled regardless of dbms fix more merge conflicts I forgot Add more env variable processing Add fix that allows you to enable the cassandra logger debug TR_PersistLogging flag Bring in changes by Ida for persistentLogger spec, dynamically load mongoc library addressed code review feedback addressed code review feedback addressed code review feedback Begin conversion of mongo to c strings and reformat to code conventions re-enable flags Reformat mongologger addressed code review feedback addressed code review feedback addressed code review feedback Address Feedback. Convert to C Strings. Cleanup Mongoc Finish making c style changes Re-add client UID setter and getter in J9PersistentInfo.cpp Dynamic Load Cassandra Fist address freedback stuff made error messages consistent Convert sprintf to snprintf Convert sprintf to snprintf Remove final debug print

mpirvu

I have some more comments on the code

mpirvu · 2020-04-07T23:57:46Z

runtime/compiler/control/HookedByTheJit.cpp

 #include "runtime/Listener.hpp"
-#endif
+#if defined(MONGO_LOGGER)
+#include "control/LoadDBLibs.hpp"


Why is "control/LoadDBLibs.hpp" needed? The only thing used in this file is the declaration for JITServer::cleanupMongoC().

mpirvu · 2020-04-07T23:59:32Z

runtime/compiler/control/J9Options.cpp

   int32_t xxJITServerSSLCertArgIndex = FIND_ARG_IN_VMARGS(STARTSWITH_MATCH, xxJITServerSSLCertOption, 0);
   int32_t xxJITServerSSLRootCertsArgIndex = FIND_ARG_IN_VMARGS(STARTSWITH_MATCH, xxJITServerSSLRootCertsOption, 0);
-
+#if defined(MONGO_LOGGER) || defined(CASSANDRA_LOGGER)


Can we group this big block protected by defined(MONGO_LOGGER) || defined(CASSANDRA_LOGGER) with the next block protected by the same ifdef

mpirvu · 2020-04-08T00:04:38Z

runtime/compiler/control/J9Options.cpp

   int32_t xxJITServerSSLRootCertsArgIndex = FIND_ARG_IN_VMARGS(STARTSWITH_MATCH, xxJITServerSSLRootCertsOption, 0);
-
+#if defined(MONGO_LOGGER) || defined(CASSANDRA_LOGGER)
+   const char *xxJITServerPersistentLoggingOption = "-XX:JITServerPersistentLogging=";


What is the meaning for the values given to this option? It appears to use an integer.
We need some comments here.
If you just wanted to turn the feature on, then we we usually use: -XX:+EnableJITServerPersistentLogging

mpirvu · 2020-04-08T00:08:41Z

runtime/compiler/control/LoadDBLibs.hpp

@@ -0,0 +1,233 @@
+//
+// Created by cmbuhler on 2020-03-11.


The file needs to have our standard license information

mpirvu · 2020-04-08T00:09:01Z

runtime/compiler/control/LoadDBLibs.cpp

@@ -0,0 +1,248 @@
+//
+// Created by cmbuhler on 2020-03-11.


All new files need to have our standard license information

mpirvu · 2020-04-08T03:06:34Z

runtime/compiler/control/MongoLogger.cpp

+   // Check if we have the database name
+   if (strcmp(_databaseName,"") == 0)
+      {
+      return "jitserver_logs";


In most cases we write something into _uri_string, but in this case we don't. For uniformity I would copy the database name into the _uri_string buffer.

mpirvu · 2020-04-08T03:15:01Z

runtime/compiler/control/MongoLogger.cpp

+   struct timespec t;
+   clock_gettime(CLOCK_REALTIME, &t);
+   int64_t timestamp = t.tv_sec * INT64_C(1000) + t.tv_nsec / 1000000;
+   char clientIDstr[20];


You may need an extra char for the terminating NULL.

mpirvu · 2020-04-08T03:17:28Z

runtime/compiler/control/MongoLogger.hpp

@@ -0,0 +1,36 @@
+#ifndef MONGOLOGGER_HPP


License information missing

mpirvu · 2020-04-08T03:18:44Z

runtime/compiler/control/rossa.cpp

         }
+
+#if defined(MONGO_LOGGER) || defined(CASSANDRA_LOGGER)
+//    TODO: Get Flag from command line.


This TODO has been implemented, hasn't it?

mpirvu · 2020-04-08T03:20:31Z

runtime/compiler/env/J9PersistentInfo.hpp

         _JITServerPort(38400),
+#if defined(MONGO_LOGGER) || defined(CASSANDRA_LOGGER)
+         _JITServerPersistentLogging(false),
+         #if defined(CASSANDRA_LOGGER)


In JIT code defines always start at first column

cmbuhler changed the title ~~Persistent logging implementation~~ WIP: Persistent logging Implementation Mar 13, 2020

mpirvu reviewed Mar 14, 2020

View reviewed changes

cmbuhler force-pushed the persistent_logging_implementation branch from 4aabc31 to 7243fad Compare March 23, 2020 02:34

cmbuhler changed the base branch from master to pre_merge March 23, 2020 02:44

cmbuhler changed the base branch from pre_merge to master March 23, 2020 02:45

cmbuhler force-pushed the persistent_logging_implementation branch from 7243fad to 565e2e8 Compare March 23, 2020 03:00

mpirvu reviewed Mar 26, 2020

View reviewed changes

cmbuhler force-pushed the persistent_logging_implementation branch from 565e2e8 to 54dd396 Compare April 3, 2020 04:55

cmbuhler mentioned this pull request Apr 3, 2020

Address feedback 2 cassandra #9

Closed

cmbuhler force-pushed the persistent_logging_implementation branch from 54dd396 to 23486ae Compare April 3, 2020 05:05

mpirvu reviewed Apr 8, 2020

View reviewed changes

WIP: Persistent logging Implementation #7

Are you sure you want to change the base?

WIP: Persistent logging Implementation #7

Uh oh!

Conversation

cmbuhler commented Mar 13, 2020

Uh oh!

mpirvu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mpirvu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels