ChinmaySKulkarni commented on a change in pull request #982:
URL: https://github.com/apache/phoenix/pull/982#discussion_r530584372
##########
File path: phoenix-core/src/it/java/org/apache/phoenix/rpc/UpdateCacheIT.java
##########
@@ -104,10 +111,30 @@ public void testUpdateCacheForNeverUpdatedTable() throws
Exception {
String tableName = generateUniqueName();
String fullTableName = INDEX_DATA_SCHEMA +
QueryConstants.NAME_SEPARATOR + tableName;
Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES);
+ AtomicBoolean isSysMutexEmpty = new AtomicBoolean(true);
+ ExecutorService executorService = Executors.newFixedThreadPool(5,
+ new ThreadFactoryBuilder().setDaemon(true)
+ .setNameFormat("check-sys-mutex-count-%d").build());
+ Connection connForMutex = DriverManager.getConnection(getUrl(), props);
+ for (int i = 0; i < 5; i++) {
+ executorService.submit(new SystemMutexCaller(isSysMutexEmpty,
Review comment:
Not a good idea to use the same phoenix connection across multiple
threads since it is not thread-safe.
##########
File path: phoenix-core/src/it/java/org/apache/phoenix/rpc/UpdateCacheIT.java
##########
@@ -104,10 +111,30 @@ public void testUpdateCacheForNeverUpdatedTable() throws
Exception {
String tableName = generateUniqueName();
String fullTableName = INDEX_DATA_SCHEMA +
QueryConstants.NAME_SEPARATOR + tableName;
Properties props = PropertiesUtil.deepCopy(TEST_PROPERTIES);
+ AtomicBoolean isSysMutexEmpty = new AtomicBoolean(true);
+ ExecutorService executorService = Executors.newFixedThreadPool(5,
+ new ThreadFactoryBuilder().setDaemon(true)
+ .setNameFormat("check-sys-mutex-count-%d").build());
+ Connection connForMutex = DriverManager.getConnection(getUrl(), props);
+ for (int i = 0; i < 5; i++) {
+ executorService.submit(new SystemMutexCaller(isSysMutexEmpty,
+ connForMutex));
+ }
+ Thread.sleep(500);
Review comment:
What is the sleep for?
##########
File path: phoenix-core/src/it/java/org/apache/phoenix/rpc/UpdateCacheIT.java
##########
@@ -269,4 +317,49 @@ public void
testInvalidConnUpdateCacheFrequencyShouldThrow() throws Exception {
}
}
}
+
+ /**
+ * Helper Runnable impl class that continuously keeps checking
+ * if SYSTEM.MUTEX contains any record until either interrupted or
+ * provided connection is closed
+ */
+ private static class SystemMutexCaller implements Runnable {
+
+ private final AtomicBoolean isSysMutexEmpty;
+ private final Connection conn;
+
+ public SystemMutexCaller(final AtomicBoolean isSysMutexEmpty,
+ final Connection conn) {
+ this.isSysMutexEmpty = isSysMutexEmpty;
+ this.conn = conn;
+ }
+
+ @Override
+ public void run() {
+ try {
+ while (!Thread.interrupted() && !conn.isClosed()) {
+ try {
+ ResultSet resultSet =
conn.createStatement().executeQuery(
+ "SELECT * FROM " +
PhoenixDatabaseMetaData.SYSTEM_MUTEX_NAME);
Review comment:
In case we _do_ end up going this route then, just to be safe, can we
query the exact expected row in SYSTEM.MUTEX instead of this query? If this
gets run in parallel with any of the upgrade tests (where we acquire an upgrade
mutex), your test will fail.
##########
File path: phoenix-core/src/it/java/org/apache/phoenix/rpc/UpdateCacheIT.java
##########
@@ -269,4 +317,49 @@ public void
testInvalidConnUpdateCacheFrequencyShouldThrow() throws Exception {
}
}
}
+
+ /**
+ * Helper Runnable impl class that continuously keeps checking
+ * if SYSTEM.MUTEX contains any record until either interrupted or
+ * provided connection is closed
+ */
+ private static class SystemMutexCaller implements Runnable {
+
+ private final AtomicBoolean isSysMutexEmpty;
+ private final Connection conn;
+
+ public SystemMutexCaller(final AtomicBoolean isSysMutexEmpty,
+ final Connection conn) {
+ this.isSysMutexEmpty = isSysMutexEmpty;
+ this.conn = conn;
+ }
+
+ @Override
+ public void run() {
+ try {
+ while (!Thread.interrupted() && !conn.isClosed()) {
+ try {
+ ResultSet resultSet =
conn.createStatement().executeQuery(
+ "SELECT * FROM " +
PhoenixDatabaseMetaData.SYSTEM_MUTEX_NAME);
+ if (resultSet.next()) {
+ isSysMutexEmpty.getAndSet(false);
Review comment:
this can be a simple `set()` since you're not using the returned value
right?
##########
File path: phoenix-core/src/it/java/org/apache/phoenix/rpc/UpdateCacheIT.java
##########
@@ -269,4 +317,49 @@ public void
testInvalidConnUpdateCacheFrequencyShouldThrow() throws Exception {
}
}
}
+
+ /**
+ * Helper Runnable impl class that continuously keeps checking
+ * if SYSTEM.MUTEX contains any record until either interrupted or
+ * provided connection is closed
+ */
+ private static class SystemMutexCaller implements Runnable {
Review comment:
Perhaps a cleaner way to test this instead of a background thread that
continuously queries SYSTEM.MUTEX is to use your own TestQueryServices impl
(some tests do this so you can add your code there) and override the
`writeMutexCell()` method to make sure if it is called. We can rely on this
method since afaik we don't do checkAndPut calls to SYSTEM.MUTEX outside of
this API
----------------------------------------------------------------
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]
