PHOENIX-4870 LoggingPhoenixConnection should log metrics when AutoCommit is set to True.#338
PHOENIX-4870 LoggingPhoenixConnection should log metrics when AutoCommit is set to True.#338swaroopak wants to merge 1 commit intoapache:4.x-HBase-1.4from
Conversation
| assertTrue("Mutation read metrics for not found for " + tableName1, | ||
| mutationReadMetricsMap.get(tableName1).size() > 0); | ||
|
|
||
| clearAllTestMetricMaps(); |
There was a problem hiding this comment.
no need to call this method unless you are checking any metrics after this.
This automatically gets called in @Before
| //with executeUpdate() method | ||
| // run SELECT to verify read metrics are logged | ||
| String query = "SELECT * FROM " + tableName1; | ||
| verifyQueryLevelMetricsLogging(query); |
There was a problem hiding this comment.
nit: Not related to this Jira, but its better to rename this method to upsertRowsAndVerifyQueryLevelMetricsLogging
| mutationWriteMetricsMap.size() > 0); | ||
| assertTrue("Mutation read metrics for not found for " + tableName1, | ||
| mutationReadMetricsMap.get(tableName1).size() > 0); | ||
| //with execute() method |
There was a problem hiding this comment.
clearAllTestMetricMaps() should be called here to ensure we are getting metrics.
| } | ||
|
|
||
| @Test | ||
| public void testPhoenixMetricsLoggedOnAutoCommit() throws Exception { |
There was a problem hiding this comment.
nit: testPhoenixMetricsLoggedOnAutoCommit to testPhoenixMetricsLoggedOnAutoCommitTrue
| loggedConn.createStatement().executeUpdate(upsertSelect); | ||
| // Autocommit is turned on explicitly | ||
| // Hence mutation metrics are expected during implicit commit | ||
| assertTrue("Mutation write metrics are not logged for " + tableName2, |
There was a problem hiding this comment.
You should check specifically for tableName2 in the map.
| private void loggingAutoCommitHelper() throws SQLException { | ||
| Connection conn = this.getConnection(); | ||
|
|
||
| if(stmt.getUpdateOperation().isMutation() && conn.getAutoCommit()){ |
There was a problem hiding this comment.
Isn't conn.getAutoCommit() check sufficient?
There was a problem hiding this comment.
Follow up: Is Statement stmt reference needed?
There was a problem hiding this comment.
This helper function gets called from both execute and executeUpdate. We need Statment reference to distinguish in case of execute
There was a problem hiding this comment.
What happens if its not a mutation and we try logging the metrics (in case of queries)?
There was a problem hiding this comment.
I see this piece of code
// If transactional, this will move the read pointer forward
if (connection.getAutoCommit()) {
connection.commit();
}
@twdsilva any idea?
There was a problem hiding this comment.
Yes, in case of executeQuery, metrics get logged. The differentiating function call happens in PhoenixStatement Class. In case of mutation, it directly executes executeMutation.
There was a problem hiding this comment.
Not sure if that is needed any more since we start the transaction in MetaDataClient.updateCache
boolean isTransactional = (table!=null && table.isTransactional());
if (isTransactional) {
connection.getMutationState().startTransaction(table.getTransactionProvider());
}
There was a problem hiding this comment.
In case of mutation, it directly executes executeMutation
You can also provide a mutation through execute() method call.
| PhoenixRuntime.getReadMetricInfoForMutationsSinceLastReset(conn)); | ||
| PhoenixRuntime.resetMetrics(conn); | ||
| } | ||
| return; |
| Connection conn = this.getConnection(); | ||
|
|
||
| if(stmt.getUpdateOperation().isMutation() && conn.getAutoCommit()){ | ||
| phoenixMetricsLog.logWriteMetricsfoForMutationsSinceLastReset( |
There was a problem hiding this comment.
Can you reuse this piece of code from somewhere?
There was a problem hiding this comment.
Unlikely, but let me check that.
There was a problem hiding this comment.
Looks like we can't.
There was a problem hiding this comment.
Can you check LoggingPhoenixConnection? It has similar calls multiple times as well. Might be good to refactor this along with it.
swaroopak
force-pushed
the
PHOENIX-4870
branch
6 times, most recently
from
74173e6 to
d2e62a9
Compare
karanmehta93
left a comment
karanmehta93
left a comment
There was a problem hiding this comment.
LGTM overall, Please fix the nits and we can commit this.
| super(stmt); | ||
| this.phoenixMetricsLog = phoenixMetricsLog; | ||
| this.sql = sql; | ||
| this.conn = conn; |
There was a problem hiding this comment.
nit: Can you move the init of variables in the same order as function arguments?
| } | ||
|
|
||
| private void loggingAutoCommitHelper() throws SQLException { | ||
| if(conn instanceof LoggingPhoenixConnection && conn.getAutoCommit()){ |
There was a problem hiding this comment.
nit:
just change the order of conditions
add brackets around conn instanceof LoggingPhoenixConnection
and add spaces before brackets in conditions (applicable everywhere)
| public LoggingPhoenixStatement(Connection conn, Statement stmt, PhoenixMetricsLog phoenixMetricsLog) { | ||
| super(stmt); | ||
| this.phoenixMetricsLog = phoenixMetricsLog; | ||
| this.conn = conn; |
|
|
||
|
|
||
| @Test | ||
| public void testPhoenixMetricsLoggedOnAutoCommitTrue() throws Exception { |
There was a problem hiding this comment.
nit: just add a description of test above as javadoc comment
swaroopak
force-pushed
the
PHOENIX-4870
branch
4 times, most recently
from
f9ef04a to
0ad3789
Compare
…mit is set to True.
swaroopak
force-pushed
the
PHOENIX-4870
branch
from
0ad3789 to
659eb41
Compare
3 participants
No description provided.