joshelser commented on a change in pull request #1210:
URL: https://github.com/apache/phoenix/pull/1210#discussion_r622210130
##########
File path:
phoenix-core/src/main/java/org/apache/phoenix/jdbc/PhoenixEmbeddedDriver.java
##########
@@ -349,7 +349,7 @@ public ConnectionInfo normalize(ReadOnlyProps props,
Properties info) throws SQL
}
if(principal == null){
if (!isConnectionless) {
- principal = props.get(QueryServices.HBASE_CLIENT_PRINCIPAL);
+ principal =
props.get(QueryServices.HBASE_CLIENT_PRINCIPAL);
Review comment:
Unnecessary?
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/log/QueryLogger.java
##########
@@ -155,6 +172,14 @@ public String getQueryId() {
public void sync(Map<String, Map<MetricType, Long>> readMetrics,
Map<MetricType, Long> overAllMetrics) {
+ syncBase(readMetrics, overAllMetrics, logLevel);
+ }
+
+ public void syncAudit(Map<String, Map<MetricType, Long>> readMetrics,
Map<MetricType, Long> overAllMetrics) {
Review comment:
Since your calls to this method are often passing `(null, null)`, how
about overloading this method to create
```
public void syncAudit() {
syncAudit(null, null, LogLevel.TRACE);
}
```
And then adopt that new call above. Would also be good to have javadoc
(especially to denote the difference between `sync` and `syncAudit`)
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/log/QueryLogger.java
##########
@@ -47,12 +48,14 @@ private QueryLogger(PhoenixConnection connection) {
this.queryId = UUID.randomUUID().toString();
this.queryDisruptor =
connection.getQueryServices().getQueryDisruptor();
logLevel = connection.getLogLevel();
+ auditLogLevel = connection.getAuditLogLevel();
Review comment:
(can't comment on this line in the code, so putting it here)
We also only get a real QueryLogger when `getLogSamplingRate()` matches the
condition inside `getInstance(...)`. Is it OK to have just one sampling rate
for both normal and audit logging?
##########
File path: phoenix-core/src/main/java/org/apache/phoenix/log/QueryLogger.java
##########
@@ -47,12 +48,14 @@ private QueryLogger(PhoenixConnection connection) {
this.queryId = UUID.randomUUID().toString();
this.queryDisruptor =
connection.getQueryServices().getQueryDisruptor();
logLevel = connection.getLogLevel();
+ auditLogLevel = connection.getAuditLogLevel();
Review comment:
We're only using `logLevel` to determine when we should instantiate a
real `QueryLogger` instance in `getInstance(PhoenixConnection, boolean)`.
Is the expectation that a user could have audit logging without "normal"
logging enabled? Right now, it looks like you have to have at least some
"normal" logging enabled to get the audit logging.
I think we would want these to be exclusive (if either normal or audit
logging is enabled, we instantiate a real QueryLogger).
##########
File path:
phoenix-core/src/main/java/org/apache/phoenix/log/QueryLogDetailsWorkHandler.java
##########
@@ -17,36 +17,29 @@
*/
package org.apache.phoenix.log;
-import java.sql.SQLException;
-
+import com.lmax.disruptor.LifecycleAware;
+import com.lmax.disruptor.WorkHandler;
import org.apache.hadoop.conf.Configuration;
-import com.lmax.disruptor.LifecycleAware;
-import com.lmax.disruptor.Sequence;
-import com.lmax.disruptor.SequenceReportingEventHandler;
+
+public class QueryLogDetailsWorkHandler implements
WorkHandler<RingBufferEvent>, LifecycleAware {
Review comment:
Trying to understand the change of type hierarchy. Seems like we didn't
actually need the Sequence part of disruptor?
--
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.
For queries about this service, please contact Infrastructure at:
[email protected]
