You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@phoenix.apache.org by xcangCRM <gi...@git.apache.org> on 2018/12/04 20:59:51 UTC
[GitHub] phoenix pull request #410: PHOENIX-5034 Log all critical statements in SYSTE...
GitHub user xcangCRM opened a pull request:
https://github.com/apache/phoenix/pull/410
PHOENIX-5034 Log all critical statements in SYSTEM.LOG table
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/xcangCRM/phoenix 4.x-HBase-1.3
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/phoenix/pull/410.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 #410
----
commit 6208a805fc2aba39c4c2ac63b21f975952fba313
Author: xcang <xc...@...>
Date: 2018-12-04T20:58:32Z
PHOENIX-5034 Log all critical statements in SYSTEM.LOG table
----
---
[GitHub] phoenix pull request #410: PHOENIX-5034 Log all critical statements in SYSTE...
Posted by karanmehta93 <gi...@git.apache.org>.
Github user karanmehta93 commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/410#discussion_r238896069
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/jdbc/PhoenixStatement.java ---
@@ -1774,11 +1778,20 @@ public QueryLogger createQueryLogger(CompilableStatement stmt, String sql) throw
}
}
}
- QueryLogger queryLogger = QueryLogger.getInstance(connection,isSystemTable);
- QueryLoggerUtil.logInitialDetails(queryLogger, connection.getTenantId(),
+
+ QueryLogger queryLogger = QueryLogger.createInstance(connection,isSystemTable,
+ isCriticalStatement(stmt));
+ QueryLoggerUtil.logInitialDetails(queryLogger, connection.getTenantId(),
connection.getQueryServices(), sql, getParameters());
+ if (isCriticalStatement(stmt)) {
+ queryLogger.sync(null, null);
+ }
return queryLogger;
}
+
+ private boolean isCriticalStatement(CompilableStatement stmt) {
--- End diff --
On a second thought, How about logging every DML that is not a "Update statement"?
It might be an overkill if we have people creating tons of views though, but we could always drop those as well.
---
[GitHub] phoenix pull request #410: PHOENIX-5034 Log all critical statements in SYSTE...
Posted by karanmehta93 <gi...@git.apache.org>.
Github user karanmehta93 commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/410#discussion_r238895706
--- Diff: phoenix-core/src/it/java/org/apache/phoenix/end2end/QueryLoggerIT.java ---
@@ -250,7 +250,103 @@ public void testWithLoggingOFF() throws Exception{
assertFalse(foundQueryLog);
conn.close();
}
-
+
+ @Test
+ //DROP Table statement should be always logged.
+ public void testDropTableStatement() throws Exception{
--- End diff --
Sorry for late comment but you can actually merge both the tests into on.
---
[GitHub] phoenix pull request #410: PHOENIX-5034 Log all critical statements in SYSTE...
Posted by karanmehta93 <gi...@git.apache.org>.
Github user karanmehta93 commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/410#discussion_r238895603
--- Diff: phoenix-core/src/it/java/org/apache/phoenix/end2end/QueryLoggerIT.java ---
@@ -250,7 +250,103 @@ public void testWithLoggingOFF() throws Exception{
assertFalse(foundQueryLog);
conn.close();
}
-
+
+ @Test
+ //DROP Table statement should be always logged.
+ public void testDropTableStatement() throws Exception{
+ String tableName = generateUniqueName();
+ createTableAndInsertValues(tableName, true);
+ Properties props= new Properties();
+ props.setProperty(QueryServices.LOG_LEVEL, LogLevel.INFO.name());
+ Connection conn = DriverManager.getConnection(getUrl(),props);
+ assertEquals(conn.unwrap(PhoenixConnection.class).getLogLevel(),LogLevel.INFO);
+
+ String query = "SELECT * FROM " + tableName;
+ ResultSet rs = conn.createStatement().executeQuery(query);
+
+ String query2 = "DROP TABLE " + tableName;
+ conn.createStatement().execute(query2);
+ while (rs.next()) {
+ rs.getString(1);
+ rs.getString(2);
+ }
+ String logQuery = "SELECT * FROM " + SYSTEM_CATALOG_SCHEMA + ".\"" + SYSTEM_LOG_TABLE
+ + "\"";
+ boolean foundQueryLog = false;
+
+ int retryCount = 0, maxRetryCount = 10;
+ int sleepTime = 1000;
+ while (retryCount++ < maxRetryCount) {
+ Thread.sleep(sleepTime);
+ try {
+ rs = conn.createStatement().executeQuery(logQuery);
+ while (rs.next()) {
+ rs.getString(QUERY);
--- End diff --
nit: not needed
---
[GitHub] phoenix pull request #410: PHOENIX-5034 Log all critical statements in SYSTE...
Posted by karanmehta93 <gi...@git.apache.org>.
Github user karanmehta93 commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/410#discussion_r238930264
--- Diff: phoenix-core/src/it/java/org/apache/phoenix/end2end/QueryLoggerIT.java ---
@@ -250,7 +250,103 @@ public void testWithLoggingOFF() throws Exception{
assertFalse(foundQueryLog);
conn.close();
}
-
+
+ @Test
+ //DROP Table statement should be always logged.
+ public void testDropTableStatement() throws Exception{
--- End diff --
Also can you test a scenario with multiple drop/alter statements issued together. What happens if the RingBuffer is full and we are trying to add to it, does it block and wait OR it drops it (in that case this is not complete)?
---
[GitHub] phoenix pull request #410: PHOENIX-5034 Log all critical statements in SYSTE...
Posted by karanmehta93 <gi...@git.apache.org>.
Github user karanmehta93 commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/410#discussion_r238896548
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/log/QueryLogger.java ---
@@ -92,9 +92,20 @@ public boolean isSynced(){
}
};
- public static QueryLogger getInstance(PhoenixConnection connection, boolean isSystemTable) {
- if (connection.getLogLevel() == LogLevel.OFF || isSystemTable || ThreadLocalRandom.current()
- .nextDouble() > connection.getLogSamplingRate()) { return NO_OP_INSTANCE; }
+ public static QueryLogger createInstance(PhoenixConnection connection, boolean isSystemTable,
+ boolean criticalStatement) {
+ // always log critical statements (DROP,ALTER for now)
+ // do not log anything when loglevel is off.
+ // do not log systemTable statement.
+ // do sampling on other statements based on configured percentage, 1% by default.
+ if (connection.getLogLevel() == LogLevel.OFF) {
--- End diff --
Good work on separating out these conditions into two different ones. It looks cleaner now.
---
[GitHub] phoenix pull request #410: PHOENIX-5034 Log all critical statements in SYSTE...
Posted by karanmehta93 <gi...@git.apache.org>.
Github user karanmehta93 commented on a diff in the pull request:
https://github.com/apache/phoenix/pull/410#discussion_r238908527
--- Diff: phoenix-core/src/main/java/org/apache/phoenix/jdbc/PhoenixStatement.java ---
@@ -1774,11 +1778,20 @@ public QueryLogger createQueryLogger(CompilableStatement stmt, String sql) throw
}
}
}
- QueryLogger queryLogger = QueryLogger.getInstance(connection,isSystemTable);
- QueryLoggerUtil.logInitialDetails(queryLogger, connection.getTenantId(),
+
+ QueryLogger queryLogger = QueryLogger.createInstance(connection,isSystemTable,
+ isCriticalStatement(stmt));
+ QueryLoggerUtil.logInitialDetails(queryLogger, connection.getTenantId(),
connection.getQueryServices(), sql, getParameters());
+ if (isCriticalStatement(stmt)) {
+ queryLogger.sync(null, null);
+ }
return queryLogger;
}
+
+ private boolean isCriticalStatement(CompilableStatement stmt) {
--- End diff --
Also, as discussed offline, we might not want to log `DropTableStatement` which are actually `DROP VIEW` since we can have lot of those.
---