[GitHub] carbondata pull request #2885: [CARBONDATA-3064] Support separate audit log

[qiuchenjian-2]

GitHub user jackylk opened a pull request:

    https://github.com/apache/carbondata/pull/2885

    [CARBONDATA-3064] Support separate audit log

    A new audit log is implemented as following:
    1. a framework is added for carbon command to record the audit log automatically, see command/package.scala
    2. Audit logs are output by Auditor.java, log4j config example is provided in Auditor.java file comment
    3. old audit log is removed
   
     - [X] Any interfaces changed?
     Internal interface change
   
     - [X] Any backward compatibility impacted?
     Yes
   
     - [X] Document update required?
    Yes
   
     - [X] Testing done
            Please provide details on
            - Whether new unit test cases have been added or why no new tests are required?
            - How it is tested? Please attach test report.
            - Is it a performance related change? Please attach the performance test report.
            - Any additional information to help reviewers in testing this change.
           
     - [X] For large changes, please consider breaking it into sub-tasks under an umbrella JIRA.
    NA


You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/jackylk/incubator-carbondata auditlog

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/carbondata/pull/2885.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #2885
   
----
commit ba8443390a5607e1bff7967645dd739d63a1c937
Author: Jacky Li <jacky.likun@...>
Date:   2018-10-31T06:49:38Z

    add framework for audit log

commit 10772381609c550a372c62b2465c52f9dfe689f7
Author: Jacky Li <jacky.likun@...>
Date:   2018-10-31T08:01:16Z

    remove old audit log

----


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2885
 
    Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder2.1/1178/



---

[qiuchenjian-2]

Github user xuchuanyin commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/2885#discussion_r229610947
 
    --- Diff: processing/src/main/java/org/apache/carbondata/processing/util/Auditor.java ---
    @@ -0,0 +1,216 @@
    +/*
    + * 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.carbondata.processing.util;
    +
    +import java.io.IOException;
    +import java.text.SimpleDateFormat;
    +import java.util.Date;
    +import java.util.Map;
    +
    +import org.apache.carbondata.common.annotations.InterfaceAudience;
    +import org.apache.carbondata.common.logging.impl.AuditLevel;
    +
    +import com.google.gson.Gson;
    +import com.google.gson.GsonBuilder;
    +import org.apache.hadoop.security.UserGroupInformation;
    +import org.apache.log4j.Logger;
    +
    +/**
    + * Audit logger.
    + * User can configure log4j to log to a separate file. For example
    + *
    + *  log4j.logger.carbon.audit=DEBUG, audit
    + *  log4j.appender.audit=org.apache.log4j.FileAppender
    + *  log4j.appender.audit.File=/opt/logs/audit.out
    + *  log4j.appender.audit.Threshold=AUDIT
    + *  log4j.appender.audit.Append=false
    + *  log4j.appender.audit.layout=org.apache.log4j.PatternLayout
    + *  log4j.appender.audit.layout.ConversionPattern=%m%n
    + */
    [e-mail]
    +public class Auditor {
    +  private static final Logger LOGGER = Logger.getLogger("carbon.audit");
    +  private static String username;
    +
    +  static {
    +    try {
    +      username = UserGroupInformation.getCurrentUser().getShortUserName();
    +    } catch (IOException e) {
    +      username = "unknown";
    +    }
    +  }
    +
    +  public static void logOperationStart(String opName, String opId) {
    +    OpStartMessage message = new OpStartMessage(opName, opId);
    +    Gson gson = new GsonBuilder().disableHtmlEscaping().create();
    --- End diff --
   
    What if the message contains quota strings? Will we escape that?


---

[qiuchenjian-2]

Github user xuchuanyin commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/2885#discussion_r229603657
 
    --- Diff: integration/spark2/src/main/scala/org/apache/spark/sql/execution/command/package.scala ---
    @@ -60,21 +63,71 @@ trait DataProcessOperation {
       def processData(sparkSession: SparkSession): Seq[Row]
     }
     
    +/**
    + * An utility that run the command with audit log
    + */
    +trait Auditable {
    +  // operation id that will be written in audit log
    +  private val operationId: String = String.valueOf(System.nanoTime())
    +
    +  // extra info to be written in audit log, set by subclass of AtomicRunnableCommand
    +  private var auditInfo: java.util.Map[String, String] = new java.util.HashMap[String, String]()
    +
    +  // holds the dbName and tableName for which this command is executed for
    +  // used for audit log, set by subclass of AtomicRunnableCommand
    +  private var table: String = _
    +
    +  // implement by subclass, return the operation name that record in audit log
    +  protected def opName: String
    +
    +  protected def opTime(startTime: Long) = s"${(System.nanoTime() - startTime) / 1000L / 1000L} ms"
    --- End diff --
   
    Why not use millisecond directly?


---

[qiuchenjian-2]

Github user xuchuanyin commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/2885#discussion_r229610053
 
    --- Diff: processing/src/main/java/org/apache/carbondata/processing/util/Auditor.java ---
    @@ -0,0 +1,216 @@
    +/*
    + * 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.carbondata.processing.util;
    +
    +import java.io.IOException;
    +import java.text.SimpleDateFormat;
    +import java.util.Date;
    +import java.util.Map;
    +
    +import org.apache.carbondata.common.annotations.InterfaceAudience;
    +import org.apache.carbondata.common.logging.impl.AuditLevel;
    +
    +import com.google.gson.Gson;
    +import com.google.gson.GsonBuilder;
    +import org.apache.hadoop.security.UserGroupInformation;
    +import org.apache.log4j.Logger;
    +
    +/**
    + * Audit logger.
    + * User can configure log4j to log to a separate file. For example
    + *
    + *  log4j.logger.carbon.audit=DEBUG, audit
    + *  log4j.appender.audit=org.apache.log4j.FileAppender
    + *  log4j.appender.audit.File=/opt/logs/audit.out
    + *  log4j.appender.audit.Threshold=AUDIT
    + *  log4j.appender.audit.Append=false
    + *  log4j.appender.audit.layout=org.apache.log4j.PatternLayout
    + *  log4j.appender.audit.layout.ConversionPattern=%m%n
    + */
    [e-mail]
    +public class Auditor {
    +  private static final Logger LOGGER = Logger.getLogger("carbon.audit");
    +  private static String username;
    +
    +  static {
    +    try {
    +      username = UserGroupInformation.getCurrentUser().getShortUserName();
    +    } catch (IOException e) {
    +      username = "unknown";
    +    }
    +  }
    +
    +  public static void logOperationStart(String opName, String opId) {
    +    OpStartMessage message = new OpStartMessage(opName, opId);
    +    Gson gson = new GsonBuilder().disableHtmlEscaping().create();
    +    String json = gson.toJson(message);
    +    LOGGER.log(AuditLevel.AUDIT, json);
    +  }
    +
    +  public static void logOperationSuccess(String opName, String opId, String table,
    +      String opTime, Map<String, String> extraInfo) {
    +    OpEndMessage message = new OpEndMessage(opName, opId, table, opTime,
    +        OpStatus.SUCCESS, extraInfo);
    +    Gson gson = new GsonBuilder().disableHtmlEscaping().create();
    +    String json = gson.toJson(message);
    +    LOGGER.log(AuditLevel.AUDIT, json);
    +  }
    +
    +  public static void logOperationFailed(String opName, String opId, String table,
    --- End diff --
   
    find many duplicated code with `logOperationSuccess`


---

[qiuchenjian-2]

Github user xuchuanyin commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/2885#discussion_r229606100
 
    --- Diff: integration/spark2/src/main/scala/org/apache/spark/sql/execution/command/partition/CarbonAlterTableAddHivePartitionCommand.scala ---
    @@ -55,6 +55,8 @@ case class CarbonAlterTableAddHivePartitionCommand(
     
       override def processMetadata(sparkSession: SparkSession): Seq[Row] = {
         table = CarbonEnv.getCarbonTable(tableName)(sparkSession)
    +    setAuditTable(table)
    +    setAuditInfo(Map("partition" -> partitionSpecsAndLocs.mkString(",")))
    --- End diff --
   
    please add an extra space after comma for better reading


---

[qiuchenjian-2]

Github user Sssan520 commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/2885#discussion_r229618208
 
    --- Diff: processing/src/main/java/org/apache/carbondata/processing/util/Auditor.java ---
    @@ -0,0 +1,216 @@
    +/*
    + * 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.carbondata.processing.util;
    +
    +import java.io.IOException;
    +import java.text.SimpleDateFormat;
    +import java.util.Date;
    +import java.util.Map;
    +
    +import org.apache.carbondata.common.annotations.InterfaceAudience;
    +import org.apache.carbondata.common.logging.impl.AuditLevel;
    +
    +import com.google.gson.Gson;
    +import com.google.gson.GsonBuilder;
    +import org.apache.hadoop.security.UserGroupInformation;
    +import org.apache.log4j.Logger;
    +
    +/**
    + * Audit logger.
    + * User can configure log4j to log to a separate file. For example
    + *
    + *  log4j.logger.carbon.audit=DEBUG, audit
    + *  log4j.appender.audit=org.apache.log4j.FileAppender
    + *  log4j.appender.audit.File=/opt/logs/audit.out
    + *  log4j.appender.audit.Threshold=AUDIT
    + *  log4j.appender.audit.Append=false
    + *  log4j.appender.audit.layout=org.apache.log4j.PatternLayout
    + *  log4j.appender.audit.layout.ConversionPattern=%m%n
    + */
    [e-mail]
    +public class Auditor {
    +  private static final Logger LOGGER = Logger.getLogger("carbon.audit");
    +  private static String username;
    +
    +  static {
    +    try {
    +      username = UserGroupInformation.getCurrentUser().getShortUserName();
    +    } catch (IOException e) {
    +      username = "unknown";
    +    }
    +  }
    +
    +  public static void logOperationStart(String opName, String opId) {
    +    OpStartMessage message = new OpStartMessage(opName, opId);
    +    Gson gson = new GsonBuilder().disableHtmlEscaping().create();
    +    String json = gson.toJson(message);
    +    LOGGER.log(AuditLevel.AUDIT, json);
    +  }
    +
    +  public static void logOperationSuccess(String opName, String opId, String table,
    +      String opTime, Map<String, String> extraInfo) {
    +    OpEndMessage message = new OpEndMessage(opName, opId, table, opTime,
    +        OpStatus.SUCCESS, extraInfo);
    +    Gson gson = new GsonBuilder().disableHtmlEscaping().create();
    --- End diff --
   
    maybe it is better to use the object gson as a member of the class Audit


---

[qiuchenjian-2]

Github user Sssan520 commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/2885#discussion_r229618246
 
    --- Diff: processing/src/main/java/org/apache/carbondata/processing/util/Auditor.java ---
    @@ -0,0 +1,216 @@
    +/*
    + * 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.carbondata.processing.util;
    +
    +import java.io.IOException;
    +import java.text.SimpleDateFormat;
    +import java.util.Date;
    +import java.util.Map;
    +
    +import org.apache.carbondata.common.annotations.InterfaceAudience;
    +import org.apache.carbondata.common.logging.impl.AuditLevel;
    +
    +import com.google.gson.Gson;
    +import com.google.gson.GsonBuilder;
    +import org.apache.hadoop.security.UserGroupInformation;
    +import org.apache.log4j.Logger;
    +
    +/**
    + * Audit logger.
    + * User can configure log4j to log to a separate file. For example
    + *
    + *  log4j.logger.carbon.audit=DEBUG, audit
    + *  log4j.appender.audit=org.apache.log4j.FileAppender
    + *  log4j.appender.audit.File=/opt/logs/audit.out
    + *  log4j.appender.audit.Threshold=AUDIT
    + *  log4j.appender.audit.Append=false
    + *  log4j.appender.audit.layout=org.apache.log4j.PatternLayout
    + *  log4j.appender.audit.layout.ConversionPattern=%m%n
    + */
    [e-mail]
    +public class Auditor {
    +  private static final Logger LOGGER = Logger.getLogger("carbon.audit");
    +  private static String username;
    +
    +  static {
    +    try {
    +      username = UserGroupInformation.getCurrentUser().getShortUserName();
    +    } catch (IOException e) {
    +      username = "unknown";
    +    }
    +  }
    +
    +  public static void logOperationStart(String opName, String opId) {
    +    OpStartMessage message = new OpStartMessage(opName, opId);
    +    Gson gson = new GsonBuilder().disableHtmlEscaping().create();
    +    String json = gson.toJson(message);
    +    LOGGER.log(AuditLevel.AUDIT, json);
    +  }
    +
    +  public static void logOperationSuccess(String opName, String opId, String table,
    +      String opTime, Map<String, String> extraInfo) {
    +    OpEndMessage message = new OpEndMessage(opName, opId, table, opTime,
    +        OpStatus.SUCCESS, extraInfo);
    +    Gson gson = new GsonBuilder().disableHtmlEscaping().create();
    +    String json = gson.toJson(message);
    +    LOGGER.log(AuditLevel.AUDIT, json);
    +  }
    +
    +  public static void logOperationFailed(String opName, String opId, String table,
    +      String opTime, Map<String, String> extraInfo) {
    +    OpEndMessage message = new OpEndMessage(opName, opId, table, opTime,
    +        OpStatus.FAILED, extraInfo);
    +    Gson gson = new GsonBuilder().disableHtmlEscaping().create();
    +    String json = gson.toJson(message);
    +    LOGGER.log(AuditLevel.AUDIT, json);
    +  }
    +
    +  private enum OpStatus {
    +    // operation started
    +    START,
    +
    +    // operation succeed
    +    SUCCESS,
    +
    +    // operation failed
    +    FAILED
    +  }
    +
    +  // log message for operation start, it is written as a JSON record in the audit log
    +  private static class OpStartMessage {
    +    private String time;
    +    private String username;
    +    private String opName;
    +    private String opId;
    +    private OpStatus opStatus;
    +
    +    OpStartMessage(String opName, String opId) {
    +      SimpleDateFormat format = new SimpleDateFormat("yyyy-MM-dd HH:mm:ss");
    --- End diff --
   
    maybe it is better to use the object format as a static member of the class OpStartMessage


---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2885
 
    Build Failed with Spark 2.2.1, Please check CI http://95.216.28.178:8080/job/ApacheCarbonPRBuilder1/1391/



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2885
 
    Build Failed  with Spark 2.3.1, Please check CI http://136.243.101.176:8080/job/carbondataprbuilder2.3/9442/



---

[qiuchenjian-2]

Github user jackylk commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/2885#discussion_r229660272
 
    --- Diff: processing/src/main/java/org/apache/carbondata/processing/util/Auditor.java ---
    @@ -0,0 +1,216 @@
    +/*
    + * 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.carbondata.processing.util;
    +
    +import java.io.IOException;
    +import java.text.SimpleDateFormat;
    +import java.util.Date;
    +import java.util.Map;
    +
    +import org.apache.carbondata.common.annotations.InterfaceAudience;
    +import org.apache.carbondata.common.logging.impl.AuditLevel;
    +
    +import com.google.gson.Gson;
    +import com.google.gson.GsonBuilder;
    +import org.apache.hadoop.security.UserGroupInformation;
    +import org.apache.log4j.Logger;
    +
    +/**
    + * Audit logger.
    + * User can configure log4j to log to a separate file. For example
    + *
    + *  log4j.logger.carbon.audit=DEBUG, audit
    + *  log4j.appender.audit=org.apache.log4j.FileAppender
    + *  log4j.appender.audit.File=/opt/logs/audit.out
    + *  log4j.appender.audit.Threshold=AUDIT
    + *  log4j.appender.audit.Append=false
    + *  log4j.appender.audit.layout=org.apache.log4j.PatternLayout
    + *  log4j.appender.audit.layout.ConversionPattern=%m%n
    + */
    [e-mail]
    +public class Auditor {
    +  private static final Logger LOGGER = Logger.getLogger("carbon.audit");
    +  private static String username;
    +
    +  static {
    +    try {
    +      username = UserGroupInformation.getCurrentUser().getShortUserName();
    +    } catch (IOException e) {
    +      username = "unknown";
    +    }
    +  }
    +
    +  public static void logOperationStart(String opName, String opId) {
    +    OpStartMessage message = new OpStartMessage(opName, opId);
    +    Gson gson = new GsonBuilder().disableHtmlEscaping().create();
    +    String json = gson.toJson(message);
    +    LOGGER.log(AuditLevel.AUDIT, json);
    +  }
    +
    +  public static void logOperationSuccess(String opName, String opId, String table,
    +      String opTime, Map<String, String> extraInfo) {
    +    OpEndMessage message = new OpEndMessage(opName, opId, table, opTime,
    +        OpStatus.SUCCESS, extraInfo);
    +    Gson gson = new GsonBuilder().disableHtmlEscaping().create();
    +    String json = gson.toJson(message);
    +    LOGGER.log(AuditLevel.AUDIT, json);
    +  }
    +
    +  public static void logOperationFailed(String opName, String opId, String table,
    +      String opTime, Map<String, String> extraInfo) {
    +    OpEndMessage message = new OpEndMessage(opName, opId, table, opTime,
    +        OpStatus.FAILED, extraInfo);
    +    Gson gson = new GsonBuilder().disableHtmlEscaping().create();
    +    String json = gson.toJson(message);
    +    LOGGER.log(AuditLevel.AUDIT, json);
    +  }
    +
    +  private enum OpStatus {
    +    // operation started
    +    START,
    +
    +    // operation succeed
    +    SUCCESS,
    +
    +    // operation failed
    +    FAILED
    +  }
    +
    +  // log message for operation start, it is written as a JSON record in the audit log
    +  private static class OpStartMessage {
    +    private String time;
    +    private String username;
    +    private String opName;
    +    private String opId;
    +    private OpStatus opStatus;
    +
    +    OpStartMessage(String opName, String opId) {
    +      SimpleDateFormat format = new SimpleDateFormat("yyyy-MM-dd HH:mm:ss");
    --- End diff --
   
    SimpleDateFormat is not thread safe, and this method maybe called in multiple thread


---

[qiuchenjian-2]

Github user Sssan520 commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/2885#discussion_r229671527
 
    --- Diff: processing/src/main/java/org/apache/carbondata/processing/util/Auditor.java ---
    @@ -0,0 +1,216 @@
    +/*
    + * 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.carbondata.processing.util;
    +
    +import java.io.IOException;
    +import java.text.SimpleDateFormat;
    +import java.util.Date;
    +import java.util.Map;
    +
    +import org.apache.carbondata.common.annotations.InterfaceAudience;
    +import org.apache.carbondata.common.logging.impl.AuditLevel;
    +
    +import com.google.gson.Gson;
    +import com.google.gson.GsonBuilder;
    +import org.apache.hadoop.security.UserGroupInformation;
    +import org.apache.log4j.Logger;
    +
    +/**
    + * Audit logger.
    + * User can configure log4j to log to a separate file. For example
    + *
    + *  log4j.logger.carbon.audit=DEBUG, audit
    + *  log4j.appender.audit=org.apache.log4j.FileAppender
    + *  log4j.appender.audit.File=/opt/logs/audit.out
    + *  log4j.appender.audit.Threshold=AUDIT
    + *  log4j.appender.audit.Append=false
    + *  log4j.appender.audit.layout=org.apache.log4j.PatternLayout
    + *  log4j.appender.audit.layout.ConversionPattern=%m%n
    + */
    [e-mail]
    +public class Auditor {
    +  private static final Logger LOGGER = Logger.getLogger("carbon.audit");
    +  private static String username;
    +
    +  static {
    +    try {
    +      username = UserGroupInformation.getCurrentUser().getShortUserName();
    +    } catch (IOException e) {
    +      username = "unknown";
    +    }
    +  }
    +
    +  public static void logOperationStart(String opName, String opId) {
    +    OpStartMessage message = new OpStartMessage(opName, opId);
    +    Gson gson = new GsonBuilder().disableHtmlEscaping().create();
    +    String json = gson.toJson(message);
    +    LOGGER.log(AuditLevel.AUDIT, json);
    +  }
    +
    +  public static void logOperationSuccess(String opName, String opId, String table,
    +      String opTime, Map<String, String> extraInfo) {
    +    OpEndMessage message = new OpEndMessage(opName, opId, table, opTime,
    +        OpStatus.SUCCESS, extraInfo);
    +    Gson gson = new GsonBuilder().disableHtmlEscaping().create();
    +    String json = gson.toJson(message);
    +    LOGGER.log(AuditLevel.AUDIT, json);
    +  }
    +
    +  public static void logOperationFailed(String opName, String opId, String table,
    +      String opTime, Map<String, String> extraInfo) {
    +    OpEndMessage message = new OpEndMessage(opName, opId, table, opTime,
    +        OpStatus.FAILED, extraInfo);
    +    Gson gson = new GsonBuilder().disableHtmlEscaping().create();
    +    String json = gson.toJson(message);
    +    LOGGER.log(AuditLevel.AUDIT, json);
    +  }
    +
    +  private enum OpStatus {
    +    // operation started
    +    START,
    +
    +    // operation succeed
    +    SUCCESS,
    +
    +    // operation failed
    +    FAILED
    +  }
    +
    +  // log message for operation start, it is written as a JSON record in the audit log
    +  private static class OpStartMessage {
    +    private String time;
    +    private String username;
    +    private String opName;
    +    private String opId;
    +    private OpStatus opStatus;
    +
    +    OpStartMessage(String opName, String opId) {
    +      SimpleDateFormat format = new SimpleDateFormat("yyyy-MM-dd HH:mm:ss");
    --- End diff --
   
    may use FastDateFormat


---

[qiuchenjian-2]

Github user jackylk commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/2885#discussion_r230028618
 
    --- Diff: integration/spark2/src/main/scala/org/apache/spark/sql/execution/command/package.scala ---
    @@ -60,21 +63,71 @@ trait DataProcessOperation {
       def processData(sparkSession: SparkSession): Seq[Row]
     }
     
    +/**
    + * An utility that run the command with audit log
    + */
    +trait Auditable {
    +  // operation id that will be written in audit log
    +  private val operationId: String = String.valueOf(System.nanoTime())
    +
    +  // extra info to be written in audit log, set by subclass of AtomicRunnableCommand
    +  private var auditInfo: java.util.Map[String, String] = new java.util.HashMap[String, String]()
    +
    +  // holds the dbName and tableName for which this command is executed for
    +  // used for audit log, set by subclass of AtomicRunnableCommand
    +  private var table: String = _
    +
    +  // implement by subclass, return the operation name that record in audit log
    +  protected def opName: String
    +
    +  protected def opTime(startTime: Long) = s"${(System.nanoTime() - startTime) / 1000L / 1000L} ms"
    --- End diff --
   
    ok


---

[qiuchenjian-2]

Github user jackylk commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/2885#discussion_r230028786
 
    --- Diff: integration/spark2/src/main/scala/org/apache/spark/sql/execution/command/partition/CarbonAlterTableAddHivePartitionCommand.scala ---
    @@ -55,6 +55,8 @@ case class CarbonAlterTableAddHivePartitionCommand(
     
       override def processMetadata(sparkSession: SparkSession): Seq[Row] = {
         table = CarbonEnv.getCarbonTable(tableName)(sparkSession)
    +    setAuditTable(table)
    +    setAuditInfo(Map("partition" -> partitionSpecsAndLocs.mkString(",")))
    --- End diff --
   
    ok


---

[qiuchenjian-2]

Github user jackylk commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/2885#discussion_r230030119
 
    --- Diff: processing/src/main/java/org/apache/carbondata/processing/util/Auditor.java ---
    @@ -0,0 +1,216 @@
    +/*
    + * 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.carbondata.processing.util;
    +
    +import java.io.IOException;
    +import java.text.SimpleDateFormat;
    +import java.util.Date;
    +import java.util.Map;
    +
    +import org.apache.carbondata.common.annotations.InterfaceAudience;
    +import org.apache.carbondata.common.logging.impl.AuditLevel;
    +
    +import com.google.gson.Gson;
    +import com.google.gson.GsonBuilder;
    +import org.apache.hadoop.security.UserGroupInformation;
    +import org.apache.log4j.Logger;
    +
    +/**
    + * Audit logger.
    + * User can configure log4j to log to a separate file. For example
    + *
    + *  log4j.logger.carbon.audit=DEBUG, audit
    + *  log4j.appender.audit=org.apache.log4j.FileAppender
    + *  log4j.appender.audit.File=/opt/logs/audit.out
    + *  log4j.appender.audit.Threshold=AUDIT
    + *  log4j.appender.audit.Append=false
    + *  log4j.appender.audit.layout=org.apache.log4j.PatternLayout
    + *  log4j.appender.audit.layout.ConversionPattern=%m%n
    + */
    [e-mail]
    +public class Auditor {
    +  private static final Logger LOGGER = Logger.getLogger("carbon.audit");
    +  private static String username;
    +
    +  static {
    +    try {
    +      username = UserGroupInformation.getCurrentUser().getShortUserName();
    +    } catch (IOException e) {
    +      username = "unknown";
    +    }
    +  }
    +
    +  public static void logOperationStart(String opName, String opId) {
    +    OpStartMessage message = new OpStartMessage(opName, opId);
    +    Gson gson = new GsonBuilder().disableHtmlEscaping().create();
    +    String json = gson.toJson(message);
    +    LOGGER.log(AuditLevel.AUDIT, json);
    +  }
    +
    +  public static void logOperationSuccess(String opName, String opId, String table,
    +      String opTime, Map<String, String> extraInfo) {
    +    OpEndMessage message = new OpEndMessage(opName, opId, table, opTime,
    +        OpStatus.SUCCESS, extraInfo);
    +    Gson gson = new GsonBuilder().disableHtmlEscaping().create();
    +    String json = gson.toJson(message);
    +    LOGGER.log(AuditLevel.AUDIT, json);
    +  }
    +
    +  public static void logOperationFailed(String opName, String opId, String table,
    --- End diff --
   
    ok, changed to logOperationEnd


---

[qiuchenjian-2]

Github user jackylk commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/2885#discussion_r230030708
 
    --- Diff: processing/src/main/java/org/apache/carbondata/processing/util/Auditor.java ---
    @@ -0,0 +1,216 @@
    +/*
    + * 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.carbondata.processing.util;
    +
    +import java.io.IOException;
    +import java.text.SimpleDateFormat;
    +import java.util.Date;
    +import java.util.Map;
    +
    +import org.apache.carbondata.common.annotations.InterfaceAudience;
    +import org.apache.carbondata.common.logging.impl.AuditLevel;
    +
    +import com.google.gson.Gson;
    +import com.google.gson.GsonBuilder;
    +import org.apache.hadoop.security.UserGroupInformation;
    +import org.apache.log4j.Logger;
    +
    +/**
    + * Audit logger.
    + * User can configure log4j to log to a separate file. For example
    + *
    + *  log4j.logger.carbon.audit=DEBUG, audit
    + *  log4j.appender.audit=org.apache.log4j.FileAppender
    + *  log4j.appender.audit.File=/opt/logs/audit.out
    + *  log4j.appender.audit.Threshold=AUDIT
    + *  log4j.appender.audit.Append=false
    + *  log4j.appender.audit.layout=org.apache.log4j.PatternLayout
    + *  log4j.appender.audit.layout.ConversionPattern=%m%n
    + */
    [e-mail]
    +public class Auditor {
    +  private static final Logger LOGGER = Logger.getLogger("carbon.audit");
    +  private static String username;
    +
    +  static {
    +    try {
    +      username = UserGroupInformation.getCurrentUser().getShortUserName();
    +    } catch (IOException e) {
    +      username = "unknown";
    +    }
    +  }
    +
    +  public static void logOperationStart(String opName, String opId) {
    +    OpStartMessage message = new OpStartMessage(opName, opId);
    +    Gson gson = new GsonBuilder().disableHtmlEscaping().create();
    --- End diff --
   
    I think gson will keep the quota string.
    In gson document, it says "By default, Gson escapes HTML characters such as < > etc. Use this option to configure Gson to pass-through HTML characters as is."


---

[qiuchenjian-2]

Github user jackylk commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/2885#discussion_r230031744
 
    --- Diff: processing/src/main/java/org/apache/carbondata/processing/util/Auditor.java ---
    @@ -0,0 +1,216 @@
    +/*
    + * 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.carbondata.processing.util;
    +
    +import java.io.IOException;
    +import java.text.SimpleDateFormat;
    +import java.util.Date;
    +import java.util.Map;
    +
    +import org.apache.carbondata.common.annotations.InterfaceAudience;
    +import org.apache.carbondata.common.logging.impl.AuditLevel;
    +
    +import com.google.gson.Gson;
    +import com.google.gson.GsonBuilder;
    +import org.apache.hadoop.security.UserGroupInformation;
    +import org.apache.log4j.Logger;
    +
    +/**
    + * Audit logger.
    + * User can configure log4j to log to a separate file. For example
    + *
    + *  log4j.logger.carbon.audit=DEBUG, audit
    + *  log4j.appender.audit=org.apache.log4j.FileAppender
    + *  log4j.appender.audit.File=/opt/logs/audit.out
    + *  log4j.appender.audit.Threshold=AUDIT
    + *  log4j.appender.audit.Append=false
    + *  log4j.appender.audit.layout=org.apache.log4j.PatternLayout
    + *  log4j.appender.audit.layout.ConversionPattern=%m%n
    + */
    [e-mail]
    +public class Auditor {
    +  private static final Logger LOGGER = Logger.getLogger("carbon.audit");
    +  private static String username;
    +
    +  static {
    +    try {
    +      username = UserGroupInformation.getCurrentUser().getShortUserName();
    +    } catch (IOException e) {
    +      username = "unknown";
    +    }
    +  }
    +
    +  public static void logOperationStart(String opName, String opId) {
    +    OpStartMessage message = new OpStartMessage(opName, opId);
    +    Gson gson = new GsonBuilder().disableHtmlEscaping().create();
    +    String json = gson.toJson(message);
    +    LOGGER.log(AuditLevel.AUDIT, json);
    +  }
    +
    +  public static void logOperationSuccess(String opName, String opId, String table,
    +      String opTime, Map<String, String> extraInfo) {
    +    OpEndMessage message = new OpEndMessage(opName, opId, table, opTime,
    +        OpStatus.SUCCESS, extraInfo);
    +    Gson gson = new GsonBuilder().disableHtmlEscaping().create();
    --- End diff --
   
    ok, seems GSON is thread safe, so I change it to static member


---

[qiuchenjian-2]

Github user jackylk commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/2885#discussion_r230031804
 
    --- Diff: processing/src/main/java/org/apache/carbondata/processing/util/Auditor.java ---
    @@ -0,0 +1,216 @@
    +/*
    + * 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.carbondata.processing.util;
    +
    +import java.io.IOException;
    +import java.text.SimpleDateFormat;
    +import java.util.Date;
    +import java.util.Map;
    +
    +import org.apache.carbondata.common.annotations.InterfaceAudience;
    +import org.apache.carbondata.common.logging.impl.AuditLevel;
    +
    +import com.google.gson.Gson;
    +import com.google.gson.GsonBuilder;
    +import org.apache.hadoop.security.UserGroupInformation;
    +import org.apache.log4j.Logger;
    +
    +/**
    + * Audit logger.
    + * User can configure log4j to log to a separate file. For example
    + *
    + *  log4j.logger.carbon.audit=DEBUG, audit
    + *  log4j.appender.audit=org.apache.log4j.FileAppender
    + *  log4j.appender.audit.File=/opt/logs/audit.out
    + *  log4j.appender.audit.Threshold=AUDIT
    + *  log4j.appender.audit.Append=false
    + *  log4j.appender.audit.layout=org.apache.log4j.PatternLayout
    + *  log4j.appender.audit.layout.ConversionPattern=%m%n
    + */
    [e-mail]
    +public class Auditor {
    +  private static final Logger LOGGER = Logger.getLogger("carbon.audit");
    +  private static String username;
    +
    +  static {
    +    try {
    +      username = UserGroupInformation.getCurrentUser().getShortUserName();
    +    } catch (IOException e) {
    +      username = "unknown";
    +    }
    +  }
    +
    +  public static void logOperationStart(String opName, String opId) {
    +    OpStartMessage message = new OpStartMessage(opName, opId);
    +    Gson gson = new GsonBuilder().disableHtmlEscaping().create();
    +    String json = gson.toJson(message);
    +    LOGGER.log(AuditLevel.AUDIT, json);
    +  }
    +
    +  public static void logOperationSuccess(String opName, String opId, String table,
    +      String opTime, Map<String, String> extraInfo) {
    +    OpEndMessage message = new OpEndMessage(opName, opId, table, opTime,
    +        OpStatus.SUCCESS, extraInfo);
    +    Gson gson = new GsonBuilder().disableHtmlEscaping().create();
    +    String json = gson.toJson(message);
    +    LOGGER.log(AuditLevel.AUDIT, json);
    +  }
    +
    +  public static void logOperationFailed(String opName, String opId, String table,
    +      String opTime, Map<String, String> extraInfo) {
    +    OpEndMessage message = new OpEndMessage(opName, opId, table, opTime,
    +        OpStatus.FAILED, extraInfo);
    +    Gson gson = new GsonBuilder().disableHtmlEscaping().create();
    +    String json = gson.toJson(message);
    +    LOGGER.log(AuditLevel.AUDIT, json);
    +  }
    +
    +  private enum OpStatus {
    +    // operation started
    +    START,
    +
    +    // operation succeed
    +    SUCCESS,
    +
    +    // operation failed
    +    FAILED
    +  }
    +
    +  // log message for operation start, it is written as a JSON record in the audit log
    +  private static class OpStartMessage {
    +    private String time;
    +    private String username;
    +    private String opName;
    +    private String opId;
    +    private OpStatus opStatus;
    +
    +    OpStartMessage(String opName, String opId) {
    +      SimpleDateFormat format = new SimpleDateFormat("yyyy-MM-dd HH:mm:ss");
    --- End diff --
   
    ok, will fix


---

[qiuchenjian-2]

Github user Sssan520 commented on a diff in the pull request:

    https://github.com/apache/carbondata/pull/2885#discussion_r230032701
 
    --- Diff: processing/src/main/java/org/apache/carbondata/processing/util/Auditor.java ---
    @@ -0,0 +1,216 @@
    +/*
    + * 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.carbondata.processing.util;
    +
    +import java.io.IOException;
    +import java.text.SimpleDateFormat;
    +import java.util.Date;
    +import java.util.Map;
    +
    +import org.apache.carbondata.common.annotations.InterfaceAudience;
    +import org.apache.carbondata.common.logging.impl.AuditLevel;
    +
    +import com.google.gson.Gson;
    +import com.google.gson.GsonBuilder;
    +import org.apache.hadoop.security.UserGroupInformation;
    +import org.apache.log4j.Logger;
    +
    +/**
    + * Audit logger.
    + * User can configure log4j to log to a separate file. For example
    + *
    + *  log4j.logger.carbon.audit=DEBUG, audit
    + *  log4j.appender.audit=org.apache.log4j.FileAppender
    + *  log4j.appender.audit.File=/opt/logs/audit.out
    + *  log4j.appender.audit.Threshold=AUDIT
    + *  log4j.appender.audit.Append=false
    + *  log4j.appender.audit.layout=org.apache.log4j.PatternLayout
    + *  log4j.appender.audit.layout.ConversionPattern=%m%n
    + */
    [e-mail]
    +public class Auditor {
    +  private static final Logger LOGGER = Logger.getLogger("carbon.audit");
    +  private static String username;
    +
    +  static {
    +    try {
    +      username = UserGroupInformation.getCurrentUser().getShortUserName();
    +    } catch (IOException e) {
    +      username = "unknown";
    +    }
    +  }
    +
    +  public static void logOperationStart(String opName, String opId) {
    +    OpStartMessage message = new OpStartMessage(opName, opId);
    +    Gson gson = new GsonBuilder().disableHtmlEscaping().create();
    +    String json = gson.toJson(message);
    +    LOGGER.log(AuditLevel.AUDIT, json);
    +  }
    +
    +  public static void logOperationSuccess(String opName, String opId, String table,
    +      String opTime, Map<String, String> extraInfo) {
    +    OpEndMessage message = new OpEndMessage(opName, opId, table, opTime,
    +        OpStatus.SUCCESS, extraInfo);
    +    Gson gson = new GsonBuilder().disableHtmlEscaping().create();
    --- End diff --
   
    > ok, seems GSON is thread safe, so I change it to static member
    yes there is a fix pr: https://github.com/google/gson/issues/63



---

[qiuchenjian-2]

Github user CarbonDataQA commented on the issue:

    https://github.com/apache/carbondata/pull/2885
 
    Build Failed with Spark 2.2.1, Please check CI http://95.216.28.178:8080/job/ApacheCarbonPRBuilder1/1433/



---