mneethiraj commented on code in PR #886: URL: https://github.com/apache/ranger/pull/886#discussion_r3019026427
########## audit-server/audit-ingestor/src/main/java/org/apache/ranger/audit/producer/kafka/AuditMessageQueue.java: ########## Review Comment: Is this necessary? init() should have suceeded before log() method is called, right? ########## audit-server/audit-ingestor/src/main/java/org/apache/ranger/audit/producer/kafka/AuditMessageQueue.java: ########## Review Comment: This would set the hostname in audit log to the name of the host running audit ingestor. This isn't correct. Ingestor shouldn't modify audit log data. ########## audit-server/audit-common/src/main/java/org/apache/ranger/audit/utils/AuditServerUtils.java: ########## @@ -0,0 +1,73 @@ +/* + * 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.ranger.audit.utils; + +import org.apache.kafka.clients.admin.Admin; +import org.apache.kafka.clients.admin.DescribeTopicsResult; +import org.apache.kafka.clients.admin.TopicDescription; +import org.apache.kafka.common.errors.UnknownTopicOrPartitionException; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import java.time.Duration; +import java.util.Collections; +import java.util.concurrent.ThreadLocalRandom; + +public class AuditServerUtils { + private static final Logger LOG = LoggerFactory.getLogger(AuditServerUtils.class); + + public boolean waitUntilTopicReady(Admin admin, String topic, Duration totalWait) throws Exception { Review Comment: This can be a static method, right? ########## audit-server/audit-common/src/main/java/org/apache/ranger/audit/server/AuditConfig.java: ########## @@ -19,17 +19,22 @@ package org.apache.ranger.audit.server; +import org.apache.commons.lang3.StringUtils; +import org.apache.hadoop.conf.Configuration; import org.apache.ranger.authorization.hadoop.config.RangerConfiguration; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import java.io.File; +import java.net.MalformedURLException; +import java.net.URL; import java.util.Properties; /** * Base configuration class for Ranger Audit Server services. * Can be extended by specific services to load their custom configuration files. */ -public class AuditConfig extends RangerConfiguration { +public class AuditConfig extends Configuration { private static final Logger LOG = LoggerFactory.getLogger(AuditConfig.class); private static volatile AuditConfig sInstance; Review Comment: Is a singleton instance necessary for AuditConfig? Consider having callers instantiate their own instance. ########## audit-server/audit-common/src/main/java/org/apache/ranger/audit/server/AuditConfig.java: ########## @@ -87,4 +92,58 @@ protected boolean addAuditResource(String resourcePath, boolean required) { return success || !required; } + + public boolean addResourceIfReadable(String aResourceName) { + LOG.debug("==> addResourceIfReadable({})", aResourceName); + + boolean ret = false; + URL fUrl = getFileLocation(aResourceName); + + if (fUrl != null) { + LOG.debug("addResourceIfReadable({}): resource file is {}", aResourceName, fUrl); + + try { + addResource(fUrl); + + ret = true; + } catch (Exception e) { + LOG.error("Unable to load the resource name [{}]. Ignoring the resource:{}", aResourceName, fUrl); + + LOG.debug("Resource loading failed for {}", fUrl, e); + } + } else { + LOG.debug("addResourceIfReadable({}): couldn't find resource file location", aResourceName); + } + + LOG.debug("<== addResourceIfReadable({}), result={}", aResourceName, ret); + + return ret; + } + + public URL getFileLocation(String fileName) { + URL lurl = null; + + if (!StringUtils.isEmpty(fileName)) { + lurl = RangerConfiguration.class.getClassLoader().getResource(fileName); Review Comment: `RangerConfiguration` => `AuditConfig` ########## audit-server/audit-common/src/main/java/org/apache/ranger/audit/utils/AuditMessageQueueUtils.java: ########## Review Comment: None of these members seem to be used. Please review and remove. - isTopicReady - isSolrConsumerEnabled - isHDFSConsumerEnabled ########## audit-server/audit-ingestor/src/main/java/org/apache/ranger/audit/producer/kafka/AuditMessageQueue.java: ########## Review Comment: Please keep these members `private`. ########## audit-server/audit-common/src/main/java/org/apache/ranger/audit/utils/AuditMessageQueueUtils.java: ########## @@ -56,15 +56,15 @@ public AuditMessageQueueUtils(Properties props) { public String createAuditsTopicIfNotExists(Properties props, String propPrefix) { LOG.info("==> AuditMessageQueueUtils:createAuditsTopicIfNotExists()"); - String ret = null; - String topicName = MiscUtil.getStringProperty(props, propPrefix + "." + AuditServerConstants.PROP_TOPIC_NAME, AuditServerConstants.DEFAULT_TOPIC); - String bootstrapServers = MiscUtil.getStringProperty(props, propPrefix + "." + AuditServerConstants.PROP_BOOTSTRAP_SERVERS); - String securityProtocol = MiscUtil.getStringProperty(props, propPrefix + "." + AuditServerConstants.PROP_SECURITY_PROTOCOL, AuditServerConstants.DEFAULT_SECURITY_PROTOCOL); - String saslMechanism = MiscUtil.getStringProperty(props, propPrefix + "." + AuditServerConstants.PROP_SASL_MECHANISM, AuditServerConstants.DEFAULT_SASL_MECHANISM); - int connMaxIdleTimeoutMS = MiscUtil.getIntProperty(props, propPrefix + "." + AuditServerConstants.PROP_CONN_MAX_IDEAL_MS, 10000); - int partitions = getPartitions(props, propPrefix); - short replicationFactor = (short) MiscUtil.getIntProperty(props, propPrefix + "." + AuditServerConstants.PROP_REPLICATION_FACTOR, AuditServerConstants.DEFAULT_REPLICATION_FACTOR); - int reqTimeoutMS = MiscUtil.getIntProperty(props, propPrefix + "." + AuditServerConstants.PROP_REQ_TIMEOUT_MS, 5000); + String ret = null; + String topicName = MiscUtil.getStringProperty(props, propPrefix + "." + AuditServerConstants.PROP_TOPIC_NAME, AuditServerConstants.DEFAULT_TOPIC); + String bootstrapServers = MiscUtil.getStringProperty(props, propPrefix + "." + AuditServerConstants.PROP_BOOTSTRAP_SERVERS); + String securityProtocol = MiscUtil.getStringProperty(props, propPrefix + "." + AuditServerConstants.PROP_SECURITY_PROTOCOL, AuditServerConstants.DEFAULT_SECURITY_PROTOCOL); + String saslMechanism = MiscUtil.getStringProperty(props, propPrefix + "." + AuditServerConstants.PROP_SASL_MECHANISM, AuditServerConstants.DEFAULT_SASL_MECHANISM); + int connMaxIdleTimeoutMS = MiscUtil.getIntProperty(props, propPrefix + "." + AuditServerConstants.PROP_CONN_MAX_IDEAL_MS, 10000); + int partitions = getPartitions(props, propPrefix); + short replicationFactor = (short) MiscUtil.getIntProperty(props, propPrefix + "." + AuditServerConstants.PROP_REPLICATION_FACTOR, AuditServerConstants.DEFAULT_REPLICATION_FACTOR); + int reqTimeoutMS = MiscUtil.getIntProperty(props, propPrefix + "." + AuditServerConstants.PROP_REQ_TIMEOUT_MS, 5000); // Retry configuration for Kafka connection during startup int maxRetries = MiscUtil.getIntProperty(props, propPrefix + "." + AuditServerConstants.PROP_KAFKA_STARTUP_MAX_RETRIES, 10); Review Comment: `PROP_KAFKA_STARTUP_MAX_RETRIES` => `PROP_KAFKA_TOPIC_INIT_MAX_RETRIES` `PROP_KAFKA_STARTUP_RETRY_DELAY_MS` => `PROP_KAFKA_TOPIC_INIT_RETRY_DELAY_MS` -- 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]
