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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
