jerryshao commented on code in PR #4575: URL: https://github.com/apache/gravitino/pull/4575#discussion_r1797544622
########## core/src/main/java/org/apache/gravitino/audit/AuditLogWriter.java: ########## @@ -0,0 +1,59 @@ +/* + * 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. + */ + +package org.apache.gravitino.audit; + +import java.util.Map; +import org.apache.gravitino.listener.api.event.Event; + +/** + * Interface for writing the audit log, which can write to different storage, such as file, + * database,mq. + */ +public interface AuditLogWriter { Review Comment: You can define this as `public interface AuditLogWriter extends Closeable` to avoid adding `close` interface. ########## core/src/main/java/org/apache/gravitino/audit/AuditLogWriter.java: ########## @@ -0,0 +1,59 @@ +/* + * 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. + */ + +package org.apache.gravitino.audit; + +import java.util.Map; +import org.apache.gravitino.listener.api.event.Event; + +/** + * Interface for writing the audit log, which can write to different storage, such as file, + * database,mq. + */ +public interface AuditLogWriter { + + /** @return formatter. */ + Formatter getFormatter(); + + /** + * Initialize the writer with the given configuration. + * + * @param properties + */ + void init(Formatter formatter, Map<String, String> properties); Review Comment: We can have properties for different writer, so my suggestion is that we can do one more interface to represent writer name like: ``` String name(); ``` For example file writer can be "file". Then this specific writer related configurations can be "gravitino.audit.writer.file.xxx", what do you think? ########## core/src/main/java/org/apache/gravitino/audit/DefaultAuditLog.java: ########## @@ -0,0 +1,69 @@ +/* + * 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. + */ + +package org.apache.gravitino.audit; + +import lombok.Builder; +import lombok.SneakyThrows; +import org.apache.gravitino.json.JsonUtils; + +/** The default implementation of the audit log. */ +@Builder +public class DefaultAuditLog implements AuditLog { + + private String user; + + private Operation operation; + + private String identifier; + + private long timestamp; + + private boolean successful; + + @Override + public String user() { + return user; + } + + @Override + public Operation operation() { + return operation; + } + + @Override + public String identifier() { + return identifier; + } + + @Override + public long timestamp() { + return timestamp; + } + + @Override + public boolean successful() { + return successful; + } Review Comment: I think it is better to define an enum to represent `SUCCESS` or `FAILURE`, also output the string not the boolean value, what do you think? Besides, instead of using JSON format for output, is it better to follow like web log format like Nginx and others, which is a standard format, what do you think? ########## core/src/main/java/org/apache/gravitino/audit/AuditLog.java: ########## @@ -0,0 +1,294 @@ +/* + * 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. + */ + +package org.apache.gravitino.audit; + +import org.apache.gravitino.listener.api.event.AlterCatalogEvent; +import org.apache.gravitino.listener.api.event.AlterCatalogFailureEvent; +import org.apache.gravitino.listener.api.event.AlterFilesetEvent; +import org.apache.gravitino.listener.api.event.AlterFilesetFailureEvent; +import org.apache.gravitino.listener.api.event.AlterMetalakeEvent; +import org.apache.gravitino.listener.api.event.AlterMetalakeFailureEvent; +import org.apache.gravitino.listener.api.event.AlterSchemaEvent; +import org.apache.gravitino.listener.api.event.AlterSchemaFailureEvent; +import org.apache.gravitino.listener.api.event.AlterTableEvent; +import org.apache.gravitino.listener.api.event.AlterTableFailureEvent; +import org.apache.gravitino.listener.api.event.AlterTopicEvent; +import org.apache.gravitino.listener.api.event.AlterTopicFailureEvent; +import org.apache.gravitino.listener.api.event.CreateCatalogEvent; +import org.apache.gravitino.listener.api.event.CreateCatalogFailureEvent; +import org.apache.gravitino.listener.api.event.CreateFilesetEvent; +import org.apache.gravitino.listener.api.event.CreateFilesetFailureEvent; +import org.apache.gravitino.listener.api.event.CreateMetalakeEvent; +import org.apache.gravitino.listener.api.event.CreateMetalakeFailureEvent; +import org.apache.gravitino.listener.api.event.CreateSchemaEvent; +import org.apache.gravitino.listener.api.event.CreateSchemaFailureEvent; +import org.apache.gravitino.listener.api.event.CreateTableEvent; +import org.apache.gravitino.listener.api.event.CreateTableFailureEvent; +import org.apache.gravitino.listener.api.event.CreateTopicEvent; +import org.apache.gravitino.listener.api.event.CreateTopicFailureEvent; +import org.apache.gravitino.listener.api.event.DropCatalogEvent; +import org.apache.gravitino.listener.api.event.DropCatalogFailureEvent; +import org.apache.gravitino.listener.api.event.DropFilesetEvent; +import org.apache.gravitino.listener.api.event.DropFilesetFailureEvent; +import org.apache.gravitino.listener.api.event.DropMetalakeEvent; +import org.apache.gravitino.listener.api.event.DropMetalakeFailureEvent; +import org.apache.gravitino.listener.api.event.DropSchemaEvent; +import org.apache.gravitino.listener.api.event.DropSchemaFailureEvent; +import org.apache.gravitino.listener.api.event.DropTableEvent; +import org.apache.gravitino.listener.api.event.DropTableFailureEvent; +import org.apache.gravitino.listener.api.event.DropTopicEvent; +import org.apache.gravitino.listener.api.event.DropTopicFailureEvent; +import org.apache.gravitino.listener.api.event.Event; +import org.apache.gravitino.listener.api.event.GetFileLocationEvent; +import org.apache.gravitino.listener.api.event.GetFileLocationFailureEvent; +import org.apache.gravitino.listener.api.event.GetPartitionEvent; +import org.apache.gravitino.listener.api.event.GetPartitionFailureEvent; +import org.apache.gravitino.listener.api.event.ListCatalogEvent; +import org.apache.gravitino.listener.api.event.ListCatalogFailureEvent; +import org.apache.gravitino.listener.api.event.ListFilesetEvent; +import org.apache.gravitino.listener.api.event.ListFilesetFailureEvent; +import org.apache.gravitino.listener.api.event.ListMetalakeEvent; +import org.apache.gravitino.listener.api.event.ListMetalakeFailureEvent; +import org.apache.gravitino.listener.api.event.ListPartitionEvent; +import org.apache.gravitino.listener.api.event.ListPartitionFailureEvent; +import org.apache.gravitino.listener.api.event.ListSchemaEvent; +import org.apache.gravitino.listener.api.event.ListSchemaFailureEvent; +import org.apache.gravitino.listener.api.event.ListTableEvent; +import org.apache.gravitino.listener.api.event.ListTableFailureEvent; +import org.apache.gravitino.listener.api.event.ListTopicEvent; +import org.apache.gravitino.listener.api.event.ListTopicFailureEvent; +import org.apache.gravitino.listener.api.event.LoadCatalogEvent; +import org.apache.gravitino.listener.api.event.LoadCatalogFailureEvent; +import org.apache.gravitino.listener.api.event.LoadFilesetEvent; +import org.apache.gravitino.listener.api.event.LoadFilesetFailureEvent; +import org.apache.gravitino.listener.api.event.LoadMetalakeEvent; +import org.apache.gravitino.listener.api.event.LoadMetalakeFailureEvent; +import org.apache.gravitino.listener.api.event.LoadSchemaEvent; +import org.apache.gravitino.listener.api.event.LoadSchemaFailureEvent; +import org.apache.gravitino.listener.api.event.LoadTableEvent; +import org.apache.gravitino.listener.api.event.LoadTableFailureEvent; +import org.apache.gravitino.listener.api.event.LoadTopicEvent; +import org.apache.gravitino.listener.api.event.LoadTopicFailureEvent; +import org.apache.gravitino.listener.api.event.PartitionExistsEvent; +import org.apache.gravitino.listener.api.event.PartitionExistsFailureEvent; +import org.apache.gravitino.listener.api.event.PurgePartitionEvent; +import org.apache.gravitino.listener.api.event.PurgePartitionFailureEvent; +import org.apache.gravitino.listener.api.event.PurgeTableEvent; +import org.apache.gravitino.listener.api.event.PurgeTableFailureEvent; + +/** The interface define unified audit log schema. */ +public interface AuditLog { Review Comment: The name `AuditInfo` is duplicated with the one we already define in Gravitino, I'm fine to use `AuditLog` to differentiate. ########## core/src/main/java/org/apache/gravitino/Configs.java: ########## @@ -342,4 +342,39 @@ private Configs() {} .stringConf() .toSequence() .createWithDefault(Collections.emptyList()); + + public static final String AUDIT_LOG_WRITER_CONFIG_PREFIX = "gravitino.audit.writer."; + + public static final String AUDIT_LOG_FILE_WRITER_DEFAULT_FILE_NAME = + "default_gravitino_audit_log"; + + public static final ConfigEntry<Boolean> AUDIT_LOG_ENABLED_CONF = + new ConfigBuilder("gravitino.audit.enable") + .doc("Gravitino audit log enable flag") + .version(ConfigConstants.VERSION_0_7_0) + .booleanConf() + .createWithDefault(false); + + public static final ConfigEntry<String> AUDIT_LOG_WRITER_CLASS_NAME = + new ConfigBuilder("gravitino.audit.writer.") + .doc("Gravitino audit log writer class name") + .version(ConfigConstants.VERSION_0_7_0) + .stringConf() + .checkValue(StringUtils::isNotBlank, ConfigConstants.NOT_BLANK_ERROR_MSG) + .createWithDefault("org.apache.gravitino.audit.DefaultFileAuditWriter"); + + public static final ConfigEntry<String> AUDIT_LOG_FORMATTER_CLASS_NAME = + new ConfigBuilder("gravitino.audit.writer.class") + .doc("Gravitino event log formatter class name") + .version(ConfigConstants.VERSION_0_7_0) + .stringConf() + .checkValue(StringUtils::isNotBlank, ConfigConstants.NOT_BLANK_ERROR_MSG) + .createWithDefault("org.apache.gravitino.audit.DefaultFormatter"); + + public static final ConfigEntry<Boolean> AUDIT_LOG_FILE_WRITER_IMMEDIATE_FLUSH = + new ConfigBuilder("gravitino.audit.writer.immediateFlush") Review Comment: Seems like this configuration is only for default file audit log writer, right? ########## core/src/main/java/org/apache/gravitino/audit/DefaultFileAuditWriter.java: ########## @@ -0,0 +1,94 @@ +/* + * 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. + */ + +package org.apache.gravitino.audit; + +import com.google.common.annotations.VisibleForTesting; +import com.google.common.base.Preconditions; +import java.io.FileNotFoundException; +import java.io.FileOutputStream; +import java.io.OutputStream; +import java.io.OutputStreamWriter; +import java.io.Writer; +import java.nio.charset.StandardCharsets; +import java.util.Map; +import org.apache.commons.lang3.StringUtils; +import org.apache.gravitino.Configs; +import org.apache.gravitino.exceptions.GravitinoRuntimeException; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * DefaultFileAuditWriter is the default implementation of AuditLogWriter, which writes audit logs + * to a file. + */ +public class DefaultFileAuditWriter implements AuditLogWriter { Review Comment: It is not good to use the name like `DefaultXXX`, you can directly use the name like `FileAuditWriter`, `StringFormatter`, etc. ########## core/src/main/java/org/apache/gravitino/audit/AuditLog.java: ########## @@ -0,0 +1,294 @@ +/* + * 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. + */ + +package org.apache.gravitino.audit; + +import org.apache.gravitino.listener.api.event.AlterCatalogEvent; +import org.apache.gravitino.listener.api.event.AlterCatalogFailureEvent; +import org.apache.gravitino.listener.api.event.AlterFilesetEvent; +import org.apache.gravitino.listener.api.event.AlterFilesetFailureEvent; +import org.apache.gravitino.listener.api.event.AlterMetalakeEvent; +import org.apache.gravitino.listener.api.event.AlterMetalakeFailureEvent; +import org.apache.gravitino.listener.api.event.AlterSchemaEvent; +import org.apache.gravitino.listener.api.event.AlterSchemaFailureEvent; +import org.apache.gravitino.listener.api.event.AlterTableEvent; +import org.apache.gravitino.listener.api.event.AlterTableFailureEvent; +import org.apache.gravitino.listener.api.event.AlterTopicEvent; +import org.apache.gravitino.listener.api.event.AlterTopicFailureEvent; +import org.apache.gravitino.listener.api.event.CreateCatalogEvent; +import org.apache.gravitino.listener.api.event.CreateCatalogFailureEvent; +import org.apache.gravitino.listener.api.event.CreateFilesetEvent; +import org.apache.gravitino.listener.api.event.CreateFilesetFailureEvent; +import org.apache.gravitino.listener.api.event.CreateMetalakeEvent; +import org.apache.gravitino.listener.api.event.CreateMetalakeFailureEvent; +import org.apache.gravitino.listener.api.event.CreateSchemaEvent; +import org.apache.gravitino.listener.api.event.CreateSchemaFailureEvent; +import org.apache.gravitino.listener.api.event.CreateTableEvent; +import org.apache.gravitino.listener.api.event.CreateTableFailureEvent; +import org.apache.gravitino.listener.api.event.CreateTopicEvent; +import org.apache.gravitino.listener.api.event.CreateTopicFailureEvent; +import org.apache.gravitino.listener.api.event.DropCatalogEvent; +import org.apache.gravitino.listener.api.event.DropCatalogFailureEvent; +import org.apache.gravitino.listener.api.event.DropFilesetEvent; +import org.apache.gravitino.listener.api.event.DropFilesetFailureEvent; +import org.apache.gravitino.listener.api.event.DropMetalakeEvent; +import org.apache.gravitino.listener.api.event.DropMetalakeFailureEvent; +import org.apache.gravitino.listener.api.event.DropSchemaEvent; +import org.apache.gravitino.listener.api.event.DropSchemaFailureEvent; +import org.apache.gravitino.listener.api.event.DropTableEvent; +import org.apache.gravitino.listener.api.event.DropTableFailureEvent; +import org.apache.gravitino.listener.api.event.DropTopicEvent; +import org.apache.gravitino.listener.api.event.DropTopicFailureEvent; +import org.apache.gravitino.listener.api.event.Event; +import org.apache.gravitino.listener.api.event.GetFileLocationEvent; +import org.apache.gravitino.listener.api.event.GetFileLocationFailureEvent; +import org.apache.gravitino.listener.api.event.GetPartitionEvent; +import org.apache.gravitino.listener.api.event.GetPartitionFailureEvent; +import org.apache.gravitino.listener.api.event.ListCatalogEvent; +import org.apache.gravitino.listener.api.event.ListCatalogFailureEvent; +import org.apache.gravitino.listener.api.event.ListFilesetEvent; +import org.apache.gravitino.listener.api.event.ListFilesetFailureEvent; +import org.apache.gravitino.listener.api.event.ListMetalakeEvent; +import org.apache.gravitino.listener.api.event.ListMetalakeFailureEvent; +import org.apache.gravitino.listener.api.event.ListPartitionEvent; +import org.apache.gravitino.listener.api.event.ListPartitionFailureEvent; +import org.apache.gravitino.listener.api.event.ListSchemaEvent; +import org.apache.gravitino.listener.api.event.ListSchemaFailureEvent; +import org.apache.gravitino.listener.api.event.ListTableEvent; +import org.apache.gravitino.listener.api.event.ListTableFailureEvent; +import org.apache.gravitino.listener.api.event.ListTopicEvent; +import org.apache.gravitino.listener.api.event.ListTopicFailureEvent; +import org.apache.gravitino.listener.api.event.LoadCatalogEvent; +import org.apache.gravitino.listener.api.event.LoadCatalogFailureEvent; +import org.apache.gravitino.listener.api.event.LoadFilesetEvent; +import org.apache.gravitino.listener.api.event.LoadFilesetFailureEvent; +import org.apache.gravitino.listener.api.event.LoadMetalakeEvent; +import org.apache.gravitino.listener.api.event.LoadMetalakeFailureEvent; +import org.apache.gravitino.listener.api.event.LoadSchemaEvent; +import org.apache.gravitino.listener.api.event.LoadSchemaFailureEvent; +import org.apache.gravitino.listener.api.event.LoadTableEvent; +import org.apache.gravitino.listener.api.event.LoadTableFailureEvent; +import org.apache.gravitino.listener.api.event.LoadTopicEvent; +import org.apache.gravitino.listener.api.event.LoadTopicFailureEvent; +import org.apache.gravitino.listener.api.event.PartitionExistsEvent; +import org.apache.gravitino.listener.api.event.PartitionExistsFailureEvent; +import org.apache.gravitino.listener.api.event.PurgePartitionEvent; +import org.apache.gravitino.listener.api.event.PurgePartitionFailureEvent; +import org.apache.gravitino.listener.api.event.PurgeTableEvent; +import org.apache.gravitino.listener.api.event.PurgeTableFailureEvent; + +/** The interface define unified audit log schema. */ +public interface AuditLog { + /** + * The user who do the operation. + * + * @return user name. + */ + String user(); + + /** + * The operation name. + * + * @return operation name. + */ + Operation operation(); + + /** + * The identifier of the resource. + * + * @return resource identifier name. + */ + String identifier(); Review Comment: What is the meaning and definition of `identifier` here? -- 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: commits-unsubscr...@gravitino.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
