[GitHub] [cassandra] ekaterinadimitrova2 commented on a change in pull request #1051: CASSANDRA-16725 - implement nodetool getauditlog

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

ekaterinadimitrova2 commented on a change in pull request #1051:
URL: https://github.com/apache/cassandra/pull/1051#discussion_r681205552



##########
File path: src/java/org/apache/cassandra/audit/AuditLogManager.java
##########
@@ -148,6 +167,8 @@ public synchronized void disableAuditLog()
         unregisterAsListener();
         IAuditLogger oldLogger = auditLogger;
         auditLogger = new NoOpAuditLogger(Collections.emptyMap());
+        // when we disable audit logging, we should also reset options so we return default ones (from cassandra.yml)

Review comment:
       ```suggestion
           // when we disable audit logging, we should also reset options so we return default ones (from cassandra.yaml)
   ```

##########
File path: src/java/org/apache/cassandra/audit/AuditLogOptions.java
##########
@@ -60,6 +300,7 @@ public String toString()
                ", block=" + block +
                ", max_queue_weight=" + max_queue_weight +
                ", max_log_size=" + max_log_size +
+               ", max_archive_retries=" + max_archive_retries +

Review comment:
       Not sure whether this change might not break someone's parsing actually? 

##########
File path: src/java/org/apache/cassandra/tools/nodetool/EnableAuditLog.java
##########
@@ -47,9 +49,37 @@
     @Option(title = "excluded_users", name = { "--excluded-users" }, description = "Comma separated list of users to be excluded for audit log. If not set the value from cassandra.yaml will be used")
     private String excluded_users = null;
 
+    @Option(title = "roll_cycle", name = {"--roll-cycle"}, description = "How often to roll the log file (MINUTELY, HOURLY, DAILY).")

Review comment:
       Do we know why those were missing at first place?

##########
File path: src/java/org/apache/cassandra/service/StorageServiceMBean.java
##########
@@ -790,8 +790,15 @@ public StageConcurrency(int corePoolSize, int maximumPoolSize)
     /** Clears the history of clients that have connected in the past **/
     void clearConnectionHistory();
     public void disableAuditLog();
-    public void enableAuditLog(String loggerName, Map<String, String> parameters, String includedKeyspaces, String excludedKeyspaces, String includedCategories, String excludedCategories, String includedUsers, String excludedUsers) throws ConfigurationException;
-    public void enableAuditLog(String loggerName, String includedKeyspaces, String excludedKeyspaces, String includedCategories, String excludedCategories, String includedUsers, String excludedUsers) throws ConfigurationException;
+    public void enableAuditLog(String loggerName, Map<String, String> parameters, String includedKeyspaces, String excludedKeyspaces, String includedCategories, String excludedCategories,
+                               String includedUsers, String excludedUsers, Integer maxArchiveRetries, Boolean block, String rollCycle,
+                               Long maxLogSize, Integer maxQueueWeight, String archiveCommand) throws ConfigurationException, IllegalStateException;
+
+    public void enableAuditLog(String loggerName, Map<String, String> parameters, String includedKeyspaces, String excludedKeyspaces, String includedCategories, String excludedCategories,
+                               String includedUsers, String excludedUsers) throws ConfigurationException, IllegalStateException;
+
+    public void enableAuditLog(String loggerName, String includedKeyspaces, String excludedKeyspaces, String includedCategories, String excludedCategories,
+                               String includedUsers, String excludedUsers) throws ConfigurationException, IllegalStateException;

Review comment:
       So we don't use in the codebase this particular method anymore but we keep it for now? Do you know the reason why those options were not added initially to be configurable from NodeTool? 
   

##########
File path: src/java/org/apache/cassandra/audit/AuditLogManager.java
##########
@@ -148,6 +167,8 @@ public synchronized void disableAuditLog()
         unregisterAsListener();
         IAuditLogger oldLogger = auditLogger;
         auditLogger = new NoOpAuditLogger(Collections.emptyMap());
+        // when we disable audit logging, we should also reset options so we return default ones (from cassandra.yml)
+        auditLogOptions = DatabaseDescriptor.getAuditLoggingOptions();

Review comment:
       Should we really return to default ones? 
   CC  @michaelsembwever 
   I feel we need to explicitly notify the users that their config (if they have provided during enable from nodetool different one than the one in cassandra.yaml) will be reset. And probably also give them the chance to save the one they have? 
   I understand that this is not change of behavior as before they didn't have the options configurable from nodetool but now when they have them, I think we need to let them know what to expect explicitly. What do you think?




-- 
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: pr-unsubscribe@cassandra.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org