richardantal commented on code in PR #1690: URL: https://github.com/apache/phoenix/pull/1690#discussion_r1407653400
########## phoenix-coprocessors/pom.xml: ########## @@ -0,0 +1,199 @@ +<?xml version="1.0" encoding="UTF-8"?> +<!-- + Licensed to the Apache Software Foundation (ASF) under one or more + contributor license agreements. See the NOTICE file distributed with + this work for additional information regarding copyright ownership. + The ASF licenses this file to You under the Apache License, Version 2.0 + (the "License"); you may not use this file except in compliance with + the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. +--> +<project xmlns="http://maven.apache.org/POM/4.0.0"; + xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"; + xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd";> + <parent> + <artifactId>phoenix</artifactId> + <groupId>org.apache.phoenix</groupId> + <version>5.2.0-SNAPSHOT</version> + </parent> + <modelVersion>4.0.0</modelVersion> + + <artifactId>phoenix-coprocessors</artifactId> + + <dependencies> + <dependency> + <groupId>org.apache.phoenix</groupId> + <artifactId>phoenix-core</artifactId> + </dependency> + + <!-- shaded thirdparty dependencies --> + <dependency> + <groupId>org.apache.phoenix.thirdparty</groupId> + <artifactId>phoenix-shaded-guava</artifactId> + </dependency> + + <!-- HBase compat dependency --> + <dependency> + <groupId>org.apache.phoenix</groupId> + <artifactId>phoenix-hbase-compat-${hbase.compat.version}</artifactId> + <optional>true</optional> + </dependency> + + <!-- Hadoop dependencies --> + <dependency> + <groupId>org.apache.hadoop</groupId> + <artifactId>hadoop-common</artifactId> + </dependency> + <dependency> + <groupId>org.apache.hadoop</groupId> + <artifactId>hadoop-yarn-api</artifactId> + </dependency> + <dependency> + <groupId>org.apache.hadoop</groupId> + <artifactId>hadoop-mapreduce-client-core</artifactId> + </dependency> + + <!-- HBase dependencies --> + <dependency> + <groupId>org.apache.hbase</groupId> + <artifactId>hbase-common</artifactId> + </dependency> + <dependency> + <groupId>org.apache.hbase</groupId> + <artifactId>hbase-mapreduce</artifactId> + </dependency> + <dependency> + <groupId>org.apache.hbase</groupId> + <artifactId>hbase-metrics</artifactId> + </dependency> + <dependency> + <groupId>org.apache.hbase</groupId> + <artifactId>hbase-server</artifactId> + </dependency> + <dependency> + <groupId>org.apache.hbase</groupId> + <artifactId>hbase-zookeeper</artifactId> + </dependency> + <dependency> + <groupId>org.apache.hbase</groupId> + <artifactId>hbase-metrics-api</artifactId> + </dependency> + <dependency> + <groupId>org.apache.hbase</groupId> + <artifactId>hbase-client</artifactId> + </dependency> + <dependency> + <groupId>org.apache.hbase</groupId> + <artifactId>hbase-protocol</artifactId> + </dependency> + <dependency> + <groupId>org.apache.hbase</groupId> + <artifactId>hbase-protocol-shaded</artifactId> + </dependency> + + <!-- HBase Adjacent Dependencies --> + <dependency> + <groupId>org.apache.zookeeper</groupId> + <artifactId>zookeeper</artifactId> + </dependency> + <dependency> + <groupId>org.apache.zookeeper</groupId> + <artifactId>zookeeper-jute</artifactId> + </dependency> + + <!-- Omid dependencies --> + <dependency> + <groupId>org.apache.omid</groupId> + <artifactId>omid-hbase-coprocessor</artifactId> + </dependency> + <dependency> + <groupId>org.apache.omid</groupId> + <artifactId>omid-commit-table</artifactId> + </dependency> + + + <!-- Other dependencies --> + <dependency> + <groupId>org.antlr</groupId> + <artifactId>antlr-runtime</artifactId> + </dependency> + <dependency> + <groupId>com.github.stephenc.findbugs</groupId> + <artifactId>findbugs-annotations</artifactId> + </dependency> + <dependency> + <groupId>org.apache.htrace</groupId> + <artifactId>htrace-core</artifactId> + </dependency> + <dependency> + <groupId>com.google.protobuf</groupId> + <artifactId>protobuf-java</artifactId> + </dependency> + <dependency> + <groupId>com.fasterxml.jackson.core</groupId> + <artifactId>jackson-annotations</artifactId> + </dependency> + <dependency> + <groupId>org.slf4j</groupId> + <artifactId>slf4j-api</artifactId> + </dependency> + <dependency> + <groupId>joda-time</groupId> + <artifactId>joda-time</artifactId> + </dependency> + <dependency> + <groupId>org.apache.commons</groupId> + <artifactId>commons-csv</artifactId> + </dependency> + <dependency> + <groupId>com.github.stephenc.findbugs</groupId> + <artifactId>findbugs-annotations</artifactId> Review Comment: This dependency is duplicated it is there in line 128 too ########## phoenix-coprocessors/src/main/java/org/apache/phoenix/coprocessor/UngroupedAggregateRegionObserver.java: ########## @@ -900,33 +896,6 @@ static List<Expression> deserializeExpressions(byte[] b) { } } - public static byte[] serialize(PTable projectedTable) { Review Comment: I see that these functions are moved to UngroupedAggregateRegionObserverConstants but i don't like that class name ########## phoenix-tests/src/it/java/org/apache/phoenix/end2end/ExplainPlanWithStatsEnabledIT.java: ########## @@ -1260,7 +1261,7 @@ private void validatePropertyOnViewIndex(String viewName, String viewIndexName, PhoenixConnection phxConn = conn.unwrap(PhoenixConnection.class); PTable viewIndex = phxConn.getTable(new PTableKey(phxConn.getTenantId(), viewIndexName)); assertEquals("USE_STATS_FOR_PARALLELIZATION property set incorrectly", useStats, - PhoenixConfigurationUtil + ScanUtil Review Comment: We can write this to one line ########## phoenix-tests/pom.xml: ########## @@ -0,0 +1,423 @@ +<?xml version="1.0" encoding="UTF-8"?> +<!-- + Licensed to the Apache Software Foundation (ASF) under one or more + contributor license agreements. See the NOTICE file distributed with + this work for additional information regarding copyright ownership. + The ASF licenses this file to You under the Apache License, Version 2.0 + (the "License"); you may not use this file except in compliance with + the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. +--> +<project xmlns="http://maven.apache.org/POM/4.0.0"; + xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"; + xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd";> + <modelVersion>4.0.0</modelVersion> + <parent> + <groupId>org.apache.phoenix</groupId> + <artifactId>phoenix</artifactId> + <version>5.2.0-SNAPSHOT</version> + </parent> + <artifactId>phoenix-tests</artifactId> + <name>Phoenix Tests</name> + <description>Phoenix Unit and Integration tests</description> + + <properties> + <top.dir>${project.basedir}/..</top.dir> + </properties> + + <build> + <plugins> + <!-- Add the ant-generated sources to the source path --> + <plugin> + <groupId>org.apache.maven.plugins</groupId> + <artifactId>maven-site-plugin</artifactId> + </plugin> + <plugin> + <groupId>org.apache.maven.plugins</groupId> + <artifactId>maven-jar-plugin</artifactId> Review Comment: I see that it was moved from the phoenix-core, correctly https://github.com/apache/phoenix/blob/5128ad0e7d95bf45dcca85f9663fd629152f8835/phoenix-core/pom.xml#L124-L154 But I am not sure if we need this maven-jar-plugin, we could check if it can be removed ########## phoenix-coprocessors/src/main/java/org/apache/hadoop/hbase/regionserver/IndexHalfStoreFileReaderGenerator.java: ########## @@ -210,7 +210,7 @@ public InternalScanner preCompact(ObserverContext<RegionCoprocessorEnvironment> InternalScanner s, ScanType scanType, CompactionLifeCycleTracker tracker, CompactionRequest request) throws IOException { - if (!IndexUtil.isLocalIndexStore(store)) { return s; } Review Comment: Why can't we keep this function in IndexUtil? ########## phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java: ########## @@ -4068,7 +4043,7 @@ protected PhoenixConnection upgradeSystemCatalogIfRequired(PhoenixConnection met altQry.executeUpdate("ALTER TABLE " + PhoenixDatabaseMetaData.SYSTEM_STATS_NAME + " SET " + TableDescriptorBuilder.SPLIT_POLICY + "='" - + SystemStatsSplitPolicy.class.getName() + "'"); + + QueryConstants.SYSTEMSTATSSPLITPOLICY_CLASSNAME + "'"); Review Comment: Please use underscore to make this variable more readable ########## phoenix-core/src/main/java/org/apache/phoenix/coprocessorclient/UngroupedAggregateRegionObserverConstants.java: ########## @@ -0,0 +1,59 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.phoenix.coprocessorclient; + +import java.io.ByteArrayOutputStream; +import java.io.DataOutputStream; +import java.io.IOException; +import java.util.List; + +import org.apache.hadoop.io.WritableUtils; +import org.apache.phoenix.coprocessor.generated.PTableProtos; +import org.apache.phoenix.expression.Expression; +import org.apache.phoenix.expression.ExpressionType; +import org.apache.phoenix.schema.PTable; +import org.apache.phoenix.schema.PTableImpl; + +public class UngroupedAggregateRegionObserverConstants { Review Comment: I think it should be called sth like UngroupedAggregateRegionObserverHelper ########## phoenix-coprocessors/pom.xml: ########## @@ -0,0 +1,199 @@ +<?xml version="1.0" encoding="UTF-8"?> +<!-- + Licensed to the Apache Software Foundation (ASF) under one or more + contributor license agreements. See the NOTICE file distributed with + this work for additional information regarding copyright ownership. + The ASF licenses this file to You under the Apache License, Version 2.0 + (the "License"); you may not use this file except in compliance with + the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. +--> +<project xmlns="http://maven.apache.org/POM/4.0.0"; + xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"; + xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd";> + <parent> + <artifactId>phoenix</artifactId> + <groupId>org.apache.phoenix</groupId> + <version>5.2.0-SNAPSHOT</version> + </parent> + <modelVersion>4.0.0</modelVersion> + + <artifactId>phoenix-coprocessors</artifactId> + + <dependencies> + <dependency> + <groupId>org.apache.phoenix</groupId> + <artifactId>phoenix-core</artifactId> + </dependency> + + <!-- shaded thirdparty dependencies --> + <dependency> + <groupId>org.apache.phoenix.thirdparty</groupId> + <artifactId>phoenix-shaded-guava</artifactId> + </dependency> + + <!-- HBase compat dependency --> + <dependency> + <groupId>org.apache.phoenix</groupId> + <artifactId>phoenix-hbase-compat-${hbase.compat.version}</artifactId> + <optional>true</optional> + </dependency> + + <!-- Hadoop dependencies --> + <dependency> + <groupId>org.apache.hadoop</groupId> + <artifactId>hadoop-common</artifactId> + </dependency> + <dependency> + <groupId>org.apache.hadoop</groupId> + <artifactId>hadoop-yarn-api</artifactId> + </dependency> + <dependency> + <groupId>org.apache.hadoop</groupId> + <artifactId>hadoop-mapreduce-client-core</artifactId> + </dependency> + + <!-- HBase dependencies --> + <dependency> + <groupId>org.apache.hbase</groupId> + <artifactId>hbase-common</artifactId> + </dependency> + <dependency> + <groupId>org.apache.hbase</groupId> + <artifactId>hbase-mapreduce</artifactId> + </dependency> + <dependency> + <groupId>org.apache.hbase</groupId> + <artifactId>hbase-metrics</artifactId> + </dependency> + <dependency> + <groupId>org.apache.hbase</groupId> + <artifactId>hbase-server</artifactId> + </dependency> + <dependency> + <groupId>org.apache.hbase</groupId> + <artifactId>hbase-zookeeper</artifactId> + </dependency> + <dependency> + <groupId>org.apache.hbase</groupId> + <artifactId>hbase-metrics-api</artifactId> + </dependency> + <dependency> + <groupId>org.apache.hbase</groupId> + <artifactId>hbase-client</artifactId> + </dependency> + <dependency> + <groupId>org.apache.hbase</groupId> + <artifactId>hbase-protocol</artifactId> + </dependency> + <dependency> + <groupId>org.apache.hbase</groupId> + <artifactId>hbase-protocol-shaded</artifactId> + </dependency> + + <!-- HBase Adjacent Dependencies --> + <dependency> + <groupId>org.apache.zookeeper</groupId> + <artifactId>zookeeper</artifactId> + </dependency> + <dependency> + <groupId>org.apache.zookeeper</groupId> + <artifactId>zookeeper-jute</artifactId> + </dependency> + + <!-- Omid dependencies --> + <dependency> + <groupId>org.apache.omid</groupId> + <artifactId>omid-hbase-coprocessor</artifactId> + </dependency> + <dependency> + <groupId>org.apache.omid</groupId> + <artifactId>omid-commit-table</artifactId> + </dependency> + + + <!-- Other dependencies --> Review Comment: The order of these dependencies are different than the it was originally in the phoenix-core, making it hard to compare. ########## phoenix-core/src/main/java/org/apache/phoenix/query/ConnectionQueryServicesImpl.java: ########## @@ -4057,7 +4032,7 @@ protected PhoenixConnection upgradeSystemCatalogIfRequired(PhoenixConnection met altQry.executeUpdate("ALTER TABLE " + PhoenixDatabaseMetaData.SYSTEM_FUNCTION + " SET " + TableDescriptorBuilder.SPLIT_POLICY + "='" - + SystemFunctionSplitPolicy.class.getName() + "',\n" + + QueryConstants.SYSTEMFUNCTIONSPLITPOLICY_CLASSNAME + "',\n" Review Comment: Please use underscore to make this variable more readable ########## phoenix-core/src/main/java/org/apache/phoenix/schema/task/Task.java: ########## @@ -62,67 +57,60 @@ justification="Not possible to avoid") public class Task { public static final Logger LOGGER = LoggerFactory.getLogger(Task.class); - private static void mutateSystemTaskTable(PhoenixConnection conn, PreparedStatement stmt, boolean accessCheckEnabled) - throws IOException { - // we need to mutate SYSTEM.TASK with HBase/login user if access is enabled. - if (accessCheckEnabled) { - User.runAsLoginUser(new PrivilegedExceptionAction<Void>() { - @Override - public Void run() throws Exception { - final RpcCall rpcContext = RpcUtil.getRpcContext(); - // setting RPC context as null so that user can be reset - try { - RpcUtil.setRpcContext(null); - stmt.execute(); - conn.commit(); - } catch (SQLException e) { - throw new IOException(e); - } finally { - // setting RPC context back to original context of the RPC - RpcUtil.setRpcContext(rpcContext); - } - return null; - } - }); - } - else { - try { - stmt.execute(); - conn.commit(); - } catch (SQLException e) { - throw new IOException(e); - } + Review Comment: I am not happy that the order of these function changed. ########## phoenix-coprocessors/src/main/java/org/apache/phoenix/coprocessor/UncoveredIndexRegionScanner.java: ########## @@ -321,10 +324,12 @@ private boolean getNextCoveredIndexRow(List<Cell> result) throws IOException { } } else { if (indexMaintainer.isUncovered()) { + long ts = indexRow.get(0).getTimestamp(); // Since we also scan the empty column for uncovered global indexes, this mean the data row // does not exist. Delete the index row if the index is an uncovered global index - indexMaintainer.deleteRowIfAgedEnough(indexRowKey, indexRow.get(0).getTimestamp(), - ageThreshold, false, region); + if (indexMaintainer.isAgedEnough(ts, ageThreshold)) { + region.delete(indexMaintainer.createDelete(indexRowKey, ts, false)); Review Comment: What are the differences between region.delete and region.batchMutate(mutations); ? ########## phoenix-core/src/main/java/org/apache/phoenix/query/QueryConstants.java: ########## @@ -302,6 +298,37 @@ enum JoinType {INNER, LEFT_OUTER} int DIVERGED_VIEW_BASE_COLUMN_COUNT = -100; int BASE_TABLE_BASE_COLUMN_COUNT = -1; + // String constants for the server side class names, so that we don't need the server jar + // on the client side + final String METADATASPLITPOLICY_CLASSNAME = "org.apache.phoenix.schema.MetaDataSplitPolicy"; Review Comment: Please use underscore to make these 4 variable more readable ########## phoenix-core/src/main/java/org/apache/phoenix/util/MetaDataUtil.java: ########## @@ -1078,16 +1031,17 @@ public static int getAutoPartitionColIndex(PTable parentTable) { return (isMultiTenant && isSalted) ? 2 : (isMultiTenant || isSalted) ? 1 : 0; } - public static String getJdbcUrl(RegionCoprocessorEnvironment env) { - String zkQuorum = env.getConfiguration().get(HConstants.ZOOKEEPER_QUORUM); - String zkClientPort = env.getConfiguration().get(HConstants.ZOOKEEPER_CLIENT_PORT, - Integer.toString(HConstants.DEFAULT_ZOOKEEPER_CLIENT_PORT)); - String zkParentNode = env.getConfiguration().get(HConstants.ZOOKEEPER_ZNODE_PARENT, - HConstants.DEFAULT_ZOOKEEPER_ZNODE_PARENT); - return PhoenixRuntime.JDBC_PROTOCOL + PhoenixRuntime.JDBC_PROTOCOL_SEPARATOR + zkQuorum - + PhoenixRuntime.JDBC_PROTOCOL_SEPARATOR + zkClientPort - + PhoenixRuntime.JDBC_PROTOCOL_SEPARATOR + zkParentNode; - } + //TODO Review Comment: Did it got copied to somewhere or what is pending here? ########## phoenix-tests/src/test/java/org/apache/phoenix/util/ServerUtilTest.java: ########## @@ -45,13 +45,13 @@ public class ServerUtilTest { @Test public void testIsHbaseNamespaceAvailableWithExistingNamespace() throws Exception { Admin mockAdmin = getMockedAdmin(); - assertTrue(ServerUtil.isHBaseNamespaceAvailable(mockAdmin, existingNamespaceOne)); + assertTrue(ClientUtil.isHBaseNamespaceAvailable(mockAdmin, existingNamespaceOne)); Review Comment: it is bit weird calling ClientUtil in ServerUtilTest ########## phoenix-core/src/main/java/org/apache/phoenix/compile/QueryCompiler.java: ########## @@ -191,7 +191,7 @@ private void verifySCN() throws SQLException { return; } long maxLookBackAgeInMillis = - BaseScannerRegionObserver.getMaxLookbackInMillis(conn.getQueryServices(). + BaseScannerRegionObserverConstants.getMaxLookbackInMillis(conn.getQueryServices(). Review Comment: there are extra unnecessary spaces here -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
