lokiore commented on code in PR #2205:
URL: https://github.com/apache/phoenix/pull/2205#discussion_r2226956101
##########
phoenix-core/src/it/java/org/apache/phoenix/coprocessor/StatisticsCollectionRunTrackerIT.java:
##########
@@ -37,14 +38,16 @@
import org.apache.phoenix.end2end.ParallelStatsEnabledIT;
import org.apache.phoenix.end2end.ParallelStatsEnabledTest;
import org.apache.phoenix.jdbc.PhoenixConnection;
+import org.apache.phoenix.jdbc.PhoenixMonitoredConnection;
import org.apache.phoenix.schema.stats.StatisticsCollectionRunTracker;
+import org.apache.phoenix.util.PropertiesUtil;
import org.apache.phoenix.util.SchemaUtil;
import org.apache.phoenix.util.TestUtil;
import org.junit.Assert;
import org.junit.Before;
import org.junit.Test;
import org.junit.experimental.categories.Category;
-
+//Passing with HA Connection
Review Comment:
If all the tests in PR here are passing then we can start removing these
comments
##########
phoenix-core/src/it/java/org/apache/phoenix/end2end/AggregateQueryWithRegionMoves2IT.java:
##########
@@ -125,7 +125,11 @@ public static synchronized void doSetup() throws Exception
{
TestScanningResultPostDummyResultCaller.class.getName());
props.put(QueryServices.PHOENIX_POST_VALID_PROCESS,
TestScanningResultPostValidResultCaller.class.getName());
- setUpTestDriver(new ReadOnlyProps(props.entrySet().iterator()));
+
if(Boolean.parseBoolean(System.getProperty("phoenix.ha.profile.active"))){
Review Comment:
Can we make `phoenix.ha.profile.active` a constant.
##########
phoenix-core/src/it/java/org/apache/phoenix/end2end/AlterTableWithViewsIT.java:
##########
@@ -1179,23 +1181,28 @@ public void testDroppingIndexedColDropsViewIndex()
throws Exception {
} catch (TableNotFoundException e) {
}
- pconn = viewConn.unwrap(PhoenixConnection.class);
+ pconn = viewConn.unwrap(PhoenixMonitoredConnection.class);
view = pconn.getTableNoCache(viewOfTable);
assertEquals("Unexpected number of indexes ", 1,
view.getIndexes().size());
assertEquals("Unexpected index ", fullNameViewIndex2 ,
view.getIndexes().get(0).getName().getString());
assertNotEquals("Dropped index should not be in view metadata ",
fullNameViewIndex1 , view.getIndexes().get(0).getName().getString());
try {
viewIndex = pconn.getTableNoCache(fullNameViewIndex1);
fail("View index should have been dropped");
- } catch (TableNotFoundException e) {
+ } catch (Exception e) {
+ if(e instanceof SQLException && e.getCause() instanceof
TableNotFoundException) {
+
+ } else if(e instanceof TableNotFoundException) {
+
Review Comment:
If these exceptions are expected, then please add a comment that this is
expected.
##########
phoenix-core/src/it/java/org/apache/phoenix/end2end/BaseViewIT.java:
##########
@@ -42,17 +43,19 @@
import org.apache.phoenix.compile.ExplainPlan;
import org.apache.phoenix.compile.ExplainPlanAttributes;
import org.apache.phoenix.jdbc.PhoenixConnection;
+import org.apache.phoenix.jdbc.PhoenixMonitoredConnection;
import org.apache.phoenix.jdbc.PhoenixPreparedStatement;
import org.apache.phoenix.jdbc.PhoenixStatement;
import org.apache.phoenix.query.KeyRange;
import org.apache.phoenix.util.MetaDataUtil;
+import org.apache.phoenix.util.PropertiesUtil;
import org.apache.phoenix.util.ScanUtil;
import org.apache.phoenix.util.SchemaUtil;
import org.junit.experimental.categories.Category;
import org.junit.runner.RunWith;
import org.junit.runners.Parameterized;
import org.junit.runners.Parameterized.Parameters;
-
+//Cant run
Review Comment:
Why this one can't be run?
##########
phoenix-core/src/it/java/org/apache/phoenix/end2end/ArrayConcatFunctionIT.java:
##########
@@ -58,7 +60,7 @@ private String initTables(Connection conn) throws Exception {
@Test
public void testArrayConcatFunctionVarchar() throws Exception {
- Connection conn = DriverManager.getConnection(getUrl());
+ Connection conn = DriverManager.getConnection(getUrl(),
PropertiesUtil.deepCopy(TEST_PROPERTIES));
Review Comment:
maybe create a class level variable id these props are not changing test
wise?
##########
phoenix-core/src/it/java/org/apache/phoenix/end2end/PreMatureTimelyAbortScanIt.java:
##########
@@ -56,11 +65,11 @@ private String getUniqueUrl() {
@Test
public void testPreMatureScannerAbortForCount() throws Exception {
-
- try (Connection conn = DriverManager.getConnection(getUniqueUrl())) {
+ System.out.println("get url pls check all" + url);
Review Comment:
clean up required here.
##########
phoenix-core/src/it/java/org/apache/phoenix/end2end/RegexBulkLoadToolIT.java:
##########
@@ -212,7 +218,7 @@ public void testImportWithIndex() throws Exception {
stmt.close();
}
- @Test
+ // @Test - ignoring local index test for HA Connection
Review Comment:
same here don't ignore for default mode
##########
phoenix-core/src/it/java/org/apache/phoenix/end2end/BackwardCompatibilityIT.java:
##########
@@ -113,18 +123,30 @@ public static synchronized Collection<MavenCoordinates>
data() throws Exception
@Before
public synchronized void doSetup() throws Exception {
tmpDir = System.getProperty("java.io.tmpdir");
- conf = HBaseConfiguration.create();
- conf.set(QueryServices.TASK_HANDLING_INTERVAL_MS_ATTRIB,
- Long.toString(Long.MAX_VALUE));
- conf.set(QueryServices.TASK_HANDLING_INITIAL_DELAY_MS_ATTRIB,
- Long.toString(Long.MAX_VALUE));
- hbaseTestUtil = new HBaseTestingUtility(conf);
- setUpConfigForMiniCluster(conf);
- conf.set(QueryServices.EXTRA_JDBC_ARGUMENTS_ATTRIB,
QueryServicesOptions.DEFAULT_EXTRA_JDBC_ARGUMENTS);
- hbaseTestUtil.startMiniCluster();
- zkQuorum = "localhost:" + hbaseTestUtil.getZkCluster().getClientPort();
- url = PhoenixRuntime.JDBC_PROTOCOL +
PhoenixRuntime.JDBC_PROTOCOL_SEPARATOR + zkQuorum;
- DriverManager.registerDriver(PhoenixDriver.INSTANCE);
+
if(Boolean.parseBoolean(System.getProperty("phoenix.ha.profile.active"))){
+ Map<String, String> props = Maps.newHashMapWithExpectedSize(1);
+ props.put(HConstants.ZOOKEEPER_ZNODE_PARENT, "/hbase-test");
+ props.put(HConstants.CLIENT_CONNECTION_REGISTRY_IMPL_CONF_KEY,
ZKConnectionInfo.ZK_REGISTRY_NAME);
+ setUpTestClusterForHA(new
ReadOnlyProps(props.entrySet().iterator()),new
ReadOnlyProps(props.entrySet().iterator()));
+ conf = getConfiguration();
+ hbaseTestUtil = getUtility();
+ zkQuorum = "localhost:" +
hbaseTestUtil.getZkCluster().getClientPort();
+ url = PhoenixRuntime.JDBC_PROTOCOL +
PhoenixRuntime.JDBC_PROTOCOL_SEPARATOR + zkQuorum;
Review Comment:
Why are we setting different props to minicluster's conf than what we are
doing in non HA mode?
##########
phoenix-core/src/it/java/org/apache/phoenix/end2end/CsvBulkLoadToolIT.java:
##########
@@ -466,7 +473,7 @@ public void testImportOneIndexTable() throws Exception {
testImportOneIndexTable("TABLE4", false);
}
- @Test
+ // @Test - Ignoring tests with local index for HA
Review Comment:
Same here uncomment it
##########
phoenix-core/src/it/java/org/apache/phoenix/end2end/CsvBulkLoadToolIT.java:
##########
@@ -421,7 +428,7 @@ public void testImportWithIndex() throws Exception {
IndexTestUtil.assertRowsForEmptyColValue(conn, "TABLE3_IDX",
QueryConstants.VERIFIED_BYTES);
}
- @Test
+// @Test - Ignoring tests with local index for HA
Review Comment:
I think these needs to be uncommented, we can put an expected failed list
for HA Mode, but for default mode it should run as it is
##########
phoenix-core/src/it/java/org/apache/phoenix/end2end/AlterTableWithViewsIT.java:
##########
@@ -73,7 +75,7 @@
import org.apache.phoenix.thirdparty.com.google.common.base.Function;
import org.apache.phoenix.thirdparty.com.google.common.collect.Lists;
-
+//Failing with HA Connection
Review Comment:
Is this test still failing?
##########
phoenix-core/src/it/java/org/apache/phoenix/end2end/ContextClassloaderIT.java:
##########
@@ -55,16 +59,19 @@ public class ContextClassloaderIT extends BaseTest {
@BeforeClass
public static synchronized void setUpBeforeClass() throws Exception {
- Configuration conf = HBaseConfiguration.create();
- setUpConfigForMiniCluster(conf);
- hbaseTestUtil = new HBaseTestingUtility(conf);
- hbaseTestUtil.startMiniCluster();
- String clientPort =
hbaseTestUtil.getConfiguration().get(QueryServices.ZOOKEEPER_PORT_ATTRIB);
- String url = JDBC_PROTOCOL_ZK + JDBC_PROTOCOL_SEPARATOR + LOCALHOST +
JDBC_PROTOCOL_SEPARATOR + clientPort
- + JDBC_PROTOCOL_TERMINATOR + PHOENIX_TEST_DRIVER_URL_PARAM;
- driver = initAndRegisterTestDriver(url, ReadOnlyProps.EMPTY_PROPS);
-
- Connection conn = DriverManager.getConnection(url);
+
if(Boolean.parseBoolean(System.getProperty("phoenix.ha.profile.active"))){
+ setUpTestClusterForHA(ReadOnlyProps.EMPTY_PROPS,
ReadOnlyProps.EMPTY_PROPS);
+ } else {
+ Configuration conf = HBaseConfiguration.create();
+ setUpConfigForMiniCluster(conf);
+ hbaseTestUtil = new HBaseTestingUtility(conf);
+ hbaseTestUtil.startMiniCluster();
+ String clientPort =
hbaseTestUtil.getConfiguration().get(QueryServices.ZOOKEEPER_PORT_ATTRIB);
+ String url = JDBC_PROTOCOL_ZK + JDBC_PROTOCOL_SEPARATOR +
LOCALHOST + JDBC_PROTOCOL_SEPARATOR + clientPort
+ + JDBC_PROTOCOL_TERMINATOR + PHOENIX_TEST_DRIVER_URL_PARAM;
+ driver = initAndRegisterTestDriver(url, ReadOnlyProps.EMPTY_PROPS);
Review Comment:
Why are we not using test driver in HA Mode?
##########
phoenix-core/src/it/java/org/apache/phoenix/end2end/PhoenixTTLToolIT.java:
##########
@@ -75,6 +74,7 @@
* Disabling this test as this works on TTL being set on View which is removed
and will be added in future.
* TODO:- To enable this test after re-enabling TTL for view for more info
check :- PHOENIX-6978
*/
+//Failing with HA Connection
Review Comment:
This test is not being run, no change needed here
##########
phoenix-core/src/it/java/org/apache/phoenix/end2end/PhoenixDriverIT.java:
##########
@@ -69,19 +76,27 @@ public class PhoenixDriverIT extends BaseTest {
@BeforeClass
public static synchronized void setUp() throws Exception {
- conf = HBaseConfiguration.create();
- hbaseTestUtil = new HBaseTestingUtility(conf);
- setUpConfigForMiniCluster(conf);
- conf.set(QueryServices.EXTRA_JDBC_ARGUMENTS_ATTRIB,
QueryServicesOptions.DEFAULT_EXTRA_JDBC_ARGUMENTS);
- hbaseTestUtil.startMiniCluster();
- // establish url and quorum. Need to use PhoenixDriver and not
PhoenixTestDriver
- zkQuorum = "localhost:" + hbaseTestUtil.getZkCluster().getClientPort();
- url = PhoenixRuntime.JDBC_PROTOCOL_ZK +
PhoenixRuntime.JDBC_PROTOCOL_SEPARATOR + zkQuorum;
- DriverManager.registerDriver(PhoenixDriver.INSTANCE);
+
if(Boolean.parseBoolean(System.getProperty("phoenix.ha.profile.active"))){
+ Map<String, String> props = Maps.newHashMapWithExpectedSize(1);
+ setUpTestClusterForHA(new
ReadOnlyProps(props.entrySet().iterator()), new
ReadOnlyProps(props.entrySet().iterator()));
+ url =
CLUSTERS.getJdbcHAUrlWithoutPrincipal()+";"+PHOENIX_TEST_DRIVER_URL_PARAM;
+ conf = getConfiguration();
+
+ } else {
+ conf = HBaseConfiguration.create();
+ hbaseTestUtil = new HBaseTestingUtility(conf);
+ setUpConfigForMiniCluster(conf);
+ conf.set(QueryServices.EXTRA_JDBC_ARGUMENTS_ATTRIB,
QueryServicesOptions.DEFAULT_EXTRA_JDBC_ARGUMENTS);
Review Comment:
why are we not passing this prop in conf for HA mode?
##########
phoenix-core-client/src/main/java/org/apache/phoenix/jdbc/ParallelPhoenixConnection.java:
##########
@@ -679,4 +693,105 @@ public void clearMetrics() {
}
context.resetMetrics();
}
+
+ @Override
+ public ConnectionQueryServices getQueryServices() throws SQLException {
+ throw new UnsupportedOperationException();
+ }
+
+ @Override
+ public PTable getTable(PTableKey key) throws SQLException {
+ throw new UnsupportedOperationException();
+ }
+
+ @Override
+ public PTable getTable(String name) throws SQLException {
+ throw new UnsupportedOperationException();
+ }
+
+ @Override
+ public PTable getTableNoCache(String name) throws SQLException {
+ throw new UnsupportedOperationException();
+ }
+
+ @Override
+ public Consistency getConsistency() throws SQLException {
+ throw new UnsupportedOperationException();
+ }
+
+ @Override
+ @Nullable
+ public PName getTenantId() throws SQLException {
+ throw new UnsupportedOperationException();
+ }
+
+ @Override
+ public MutationState getMutationState() throws SQLException {
+ throw new UnsupportedOperationException();
+ }
+
+ @Override
+ public PMetaData getMetaDataCache() throws SQLException {
+ throw new UnsupportedOperationException();
+ }
+
+ @Override
+ public int getMutateBatchSize() throws SQLException {
+ throw new UnsupportedOperationException();
+ }
+
+ @Override
+ public int executeStatements(Reader reader, List<Object> binds,
PrintStream out) throws IOException, SQLException {
+ throw new UnsupportedOperationException();
+ }
+
+ @Override
+ public Format getFormatter(PDataType type) throws SQLException {
+ throw new UnsupportedOperationException();
+ }
+
+ @Override
+ public void setRunningUpgrade(boolean isRunningUpgrade) throws
SQLException {
+ throw new UnsupportedOperationException();
+ }
+
+ @Override
+ public PTable getTable(String tenantId, String fullTableName) throws
SQLException {
+ throw new UnsupportedOperationException();
+ }
+
+ @Override
+ public PTable getTableNoCache(PName tenantId, String name) throws
SQLException {
+ throw new UnsupportedOperationException();
+ }
+
+ @Override
+ public void setIsClosing(boolean imitateIsClosing) throws SQLException {
+ throw new UnsupportedOperationException();
+ }
+
+ @Override
+ public String getDatePattern() throws SQLException {
+ throw new UnsupportedOperationException();
+ }
+
+ @Override
+ public PTable getTable(@Nullable String tenantId, String fullTableName,
@Nullable Long timestamp) throws SQLException {
+ throw new UnsupportedOperationException();
+ }
+
+ @Override
+ public boolean isRunningUpgrade() throws SQLException {
+ throw new UnsupportedOperationException();
+ }
+
+ @Override
+ public String getURL() throws SQLException {
+ throw new UnsupportedOperationException();
+ }
+
+ @Override
+ public LogLevel getLogLevel() throws SQLException {
+ throw new UnsupportedOperationException();
Review Comment:
Can we use runOnConnections() for these methods, we are not using parallel
policy for tests but if we want to then it will be helpful?
##########
phoenix-core/src/it/java/org/apache/phoenix/end2end/MutationStateIT.java:
##########
@@ -48,21 +49,17 @@
import org.apache.phoenix.exception.SQLExceptionCode;
import org.apache.phoenix.execute.MutationState;
import org.apache.phoenix.jdbc.PhoenixConnection;
+import org.apache.phoenix.jdbc.PhoenixMonitoredConnection;
import org.apache.phoenix.query.QueryServices;
import org.apache.phoenix.schema.PIndexState;
import org.apache.phoenix.schema.PTableType;
-import org.apache.phoenix.util.PhoenixRuntime;
-import org.apache.phoenix.util.Repeat;
-import org.apache.phoenix.util.RunUntilFailure;
-import org.apache.phoenix.util.SchemaUtil;
-import org.apache.phoenix.util.StringUtil;
-import org.apache.phoenix.util.TestUtil;
+import org.apache.phoenix.util.*;
Review Comment:
Please don't use * imports
##########
phoenix-core/src/it/java/org/apache/phoenix/end2end/AppendOnlySchemaIT.java:
##########
@@ -62,20 +62,18 @@
import org.junit.experimental.categories.Category;
import org.mockito.AdditionalMatchers;
import org.mockito.Mockito;
-
+//Failing with HA Connection
Review Comment:
Same for this, does this PR include passing and failing tests or just
passing tests?
##########
phoenix-core/src/it/java/org/apache/phoenix/end2end/ConnectionIT.java:
##########
@@ -56,29 +59,37 @@ public class ConnectionIT {
@BeforeClass
public static synchronized void setUp() throws Exception {
- hbaseTestUtil = new HBaseTestingUtility();
- conf = hbaseTestUtil.getConfiguration();
- setUpConfigForMiniCluster(conf);
- conf.set(HConstants.ZOOKEEPER_ZNODE_PARENT, "/hbase-test");
- conf.set(HConstants.CLIENT_CONNECTION_REGISTRY_IMPL_CONF_KEY,
ZKConnectionInfo.ZK_REGISTRY_NAME);
- hbaseTestUtil.startMiniCluster();
- Class.forName(PhoenixDriver.class.getName());
- DriverManager.registerDriver(new PhoenixTestDriver());
- InstanceResolver.clearSingletons();
- // Make sure the ConnectionInfo doesn't try to pull a default
Configuration
- InstanceResolver.getSingleton(ConfigurationFactory.class, new
ConfigurationFactory() {
- @Override
- public Configuration getConfiguration() {
- return new Configuration(conf);
- }
-
- @Override
- public Configuration getConfiguration(Configuration confToClone) {
- Configuration copy = new Configuration(conf);
- copy.addResource(confToClone);
- return copy;
- }
- });
+
if(Boolean.parseBoolean(System.getProperty("phoenix.ha.profile.active"))){
+ Map<String, String> props = Maps.newHashMapWithExpectedSize(1);
+ props.put(HConstants.ZOOKEEPER_ZNODE_PARENT, "/hbase-test");
+ props.put(HConstants.CLIENT_CONNECTION_REGISTRY_IMPL_CONF_KEY,
ZKConnectionInfo.ZK_REGISTRY_NAME);
+ setUpTestClusterForHA(new
ReadOnlyProps(props.entrySet().iterator()),new
ReadOnlyProps(props.entrySet().iterator()));
+ conf = getConfiguration();
+ } else {
+ hbaseTestUtil = new HBaseTestingUtility();
+ conf = hbaseTestUtil.getConfiguration();
+ setUpConfigForMiniCluster(conf);
+ conf.set(HConstants.ZOOKEEPER_ZNODE_PARENT, "/hbase-test");
+ conf.set(HConstants.CLIENT_CONNECTION_REGISTRY_IMPL_CONF_KEY,
ZKConnectionInfo.ZK_REGISTRY_NAME);
+ hbaseTestUtil.startMiniCluster();
+ Class.forName(PhoenixDriver.class.getName());
+ DriverManager.registerDriver(new PhoenixTestDriver());
+ InstanceResolver.clearSingletons();
+ // Make sure the ConnectionInfo doesn't try to pull a default
Configuration
+ InstanceResolver.getSingleton(ConfigurationFactory.class, new
ConfigurationFactory() {
+ @Override
+ public Configuration getConfiguration() {
+ return new Configuration(conf);
+ }
+
+ @Override
+ public Configuration getConfiguration(Configuration
confToClone) {
+ Configuration copy = new Configuration(conf);
+ copy.addResource(confToClone);
+ return copy;
+ }
+ });
Review Comment:
Do we need to add this into HA mode as well?
--
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.
To unsubscribe, e-mail: issues-unsubscr...@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
