-
Notifications
You must be signed in to change notification settings - Fork 994
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
PHOENIX-6192 : Use tenant connection to resolve tenant views in syncUpdateCacheFreqAllIndexes() #928
Conversation
💔 -1 overall
This message was automatically generated. |
long cacheFreq = rs.getLong(1); | ||
assertEquals(0, cacheFreq); | ||
|
||
updateCacheFreq(conn, null, TABLE_NAME, 500); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: maybe use tableNameFreqVal instead of entering a numerical value?
import static org.junit.Assert.assertEquals; | ||
import static org.junit.Assert.assertNull; | ||
|
||
public class GlobalConnectionTenantTable2IT extends BaseTest { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just wondering, can these tests can't be added to GlobalConnectionTenantTableIT
itself?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In case the answer is no for some reason, can we rename this test class to be more representative of the actual test i.e. all tests are related to syncUpdateCacheFreq so maybe rename it to show that
} | ||
} | ||
|
||
private void updateCacheFreq(Connection conn, String tenantId, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Maybe this method should be called updateUpdateCacheFreq()
;)
"SELECT UPDATE_CACHE_FREQUENCY FROM SYSTEM.CATALOG WHERE TABLE_NAME='" | ||
+ TABLE_NAME + "'"); | ||
rs.next(); | ||
long cacheFreq = rs.getLong(1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Not sure we need to assign rs.get..()
to a variable each time for tenantId
, ucf
, etc. Up to you if you think it's good for clarity (I don't feel strongly about it).
assertEquals(500, cacheFreq); | ||
|
||
rs = conn.createStatement().executeQuery( | ||
"SELECT TENANT_ID,TABLE_SCHEM,UPDATE_CACHE_FREQUENCY FROM SYSTEM.CATALOG" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extract this query into a private final String
member variable to avoid duplication
updateCacheFreq(conn, TENANT_NAME, VIEW1_NAME, 999); | ||
updateCacheFreq(conn, TENANT_NAME, VIEW1_INDEX_NAME, 888); | ||
|
||
rs = conn.createStatement().executeQuery( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: these 4-5 steps are repeated multiple times so maybe worth extracting to a small helper method..something like assertUpdateCacheFreqValue()
)
cacheFreq = rs.getLong(3); | ||
assertEquals(TENANT_NAME,tenantId); | ||
assertEquals(SCHEMA_NAME, schemaName); | ||
assertEquals(500, cacheFreq); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly, shouldn't this be 0 since VIEW2 has 0 set?
try (Connection conn = getTenantConnection(TENANT_NAME)) { | ||
createView(conn, SCHEMA_NAME, VIEW1_NAME, TABLE_NAME); | ||
createViewIndex(conn, SCHEMA_NAME, VIEW1_INDEX_NAME, VIEW1_NAME, VIEW_INDEX_COL); | ||
createView(conn, SCHEMA_NAME, VIEW2_NAME, VIEW1_NAME); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be better to try out a couple of different TENANT_IDs to make sure that syncUpdateCacheFreqAllIndexes()
changes the tenant_id to use as per each view that is found
throws SQLException { | ||
String viewName = SchemaUtil.getTableName(tableInfo.getSchemaName(), | ||
tableInfo.getTableName()); | ||
String viewTenantId = Bytes.toString(tableInfo.getTenantId()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will be better to resolve the PTable for the view (either using tenanted connection if required or a global connection if a global view) and then call syncUpdateCacheFreqForIndexesOfTable()
just once. Basically, combine this method and getViewAndSyncCacheFreqForIndexes()
.
@@ -1400,6 +1393,43 @@ public static void syncUpdateCacheFreqAllIndexes(PhoenixConnection conn, PTable | |||
} | |||
} | |||
|
|||
private static void iterateOverChildViewAndSyncCacheFreq( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rename this method since we aren't iterating over anything. Maybe after combining this with getViewAndSyncCacheFreqForIndexes()
we can just call it the latter.
view = PhoenixRuntime.getTable(conn, viewName); | ||
} catch (TableNotFoundException e) { | ||
// Ignore | ||
LOGGER.warn("Error getting PTable for view: {}", viewName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be ERROR
level
cacheFreq = rs.getLong(1); | ||
assertEquals(0, cacheFreq); | ||
|
||
updateCacheFreq(conn, TENANT_NAME, VIEW1_NAME, 999); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: maybe use viewName1FreqVal instead of entering a numerical value?
@virajjasani thanks for the patch. Can you rebase and solve the conflict? |
…pdateCacheFreqAllIndexes()
💔 -1 overall
This message was automatically generated. |
Thanks for the review @ChinmaySKulkarni @yanxinyi . I have addressed your comments, could you please take a look? |
💔 -1 overall
This message was automatically generated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1. Thanks @virajjasani
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for the patch
No description provided.