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-6053 Split Server Side Code into a Separate Module #1690
Conversation
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.
Thank you.
I've left some general comments.
We can talk them over tomorrow.
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
*/ | ||
package org.apache.phoenix.coprocessor; |
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.
Having a .coprocessor. package in phoenix-core is iffy.
I'd prefer if we renamed this package to something else, like org.apache.phoenix.scanattributes or similar.
@@ -239,18 +243,50 @@ private ParallelIteratorFactory wrapParallelIteratorFactory () { | |||
// wrap any existing parallelIteratorFactory | |||
return new WrappingResultIteratorFactory(innerFactory, parallelIteratorFactory); | |||
} | |||
|
|||
|
|||
// TODO better place? |
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.
As it is only called from this class place, having it here is fine.
It doesn't need to be static either.
The same goes for other static helpers that were moved out of the coprocessors.
@@ -18,6 +18,7 @@ | |||
package org.apache.phoenix.execute; | |||
|
|||
import static org.apache.phoenix.query.QueryConstants.UNGROUPED_AGG_ROW_KEY; | |||
import static org.apache.phoenix.coprocessor.BaseScannerRegionObserverConstants.AGGREGATORS; |
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 would be nice to be consistent with static imports, but there is no need to clean that up in this patch.
@@ -128,6 +131,30 @@ public ScanPlan(StatementContext context, FilterableStatement statement, TableRe | |||
this.rowOffset = rowOffset; | |||
} | |||
|
|||
// TODO better place? | |||
public static void serializeScanRegionObserverIntoScan(Scan scan, int limit, |
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.
Maybe add a comment that it is static because it's called from tests
@@ -17,7 +17,6 @@ | |||
*/ | |||
package org.apache.phoenix.hbase.index.builder; |
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.
move to org.apache.phoenix.index ?
import org.apache.phoenix.schema.PTableImpl; | ||
import org.apache.phoenix.util.TrustedByteArrayOutputStream; | ||
|
||
public final class PhoenixIndexBuilderStatic { |
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.
The name is probably from my POC , but it would better to call this something like ...Client or ...Helper, or similary.
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.
Thee same goes for all ...Static classes.
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.
Yes, I used those names.,I'll rename them
throw new IOException(e); | ||
} | ||
|
||
// private static List<Mutation> getMutationsForSystemTaskTable( |
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 the final version remove code instead of commenting it out.
@@ -21,7 +21,6 @@ | |||
import org.apache.phoenix.thirdparty.com.google.common.collect.Maps; | |||
import org.apache.phoenix.thirdparty.com.google.common.collect.Sets; | |||
|
|||
import org.apache.hadoop.hbase.client.Delete; |
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.
I can see a lot of files where you are just randomly removing unused imports.
This change is already huge, leaving these alone for now would make it somewhat more managable.
We can handle unrelated cleanups in separate tickets.
This only applies when you only remove non-server unused imports.
When there are other changes, it's fine to clean up them.
import org.apache.phoenix.exception.SQLExceptionInfo; | ||
import org.apache.phoenix.schema.StaleRegionBoundaryCacheException; | ||
|
||
public class ClientUtil { |
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.
When splitting classes to client and server ones, check where it used more often.
In some cases it may beneficial to keep the old name for the client class, and in some cases it may be better to keep it for the server class, whichever makes more sense, or makes the patch smaller.
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.
Checked these, if I'm correct, there are 3 of these kind of splits:
- ClientUtil - ServerUtil: Here the original class was ServerUtil, so using ClientUtil on the client side is ok IMO.
- Task - ServerTask and ViewUtil - ServerViewUtil: They are similar, there is inheritance between these classes, Server versions are the more specified classes and the name represents this (also they are less used)
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.
5d6a009
to
f3af6de
Compare
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.
Left some suggestions about names and packages.
Please do another pass to undo the rest of the changes that only affect import order or unused imports.
phoenix-coprocessors/pom.xml
Outdated
<artifactId>maven-dependency-plugin</artifactId> | ||
<configuration> | ||
<ignoreNonCompile>true</ignoreNonCompile> | ||
<!-- <ignoredUnusedDeclaredDependencies> --> |
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.
You can delete these
phoenix-coprocessors/pom.xml
Outdated
</configuration> | ||
<executions> | ||
<execution> | ||
<!-- generates the file that will be used by the sandbox script |
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.
Please check that the sandbox environment still works.
import org.apache.phoenix.exception.SQLExceptionInfo; | ||
import org.apache.phoenix.schema.StaleRegionBoundaryCacheException; | ||
|
||
public class ClientUtil { |
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.
@@ -443,16 +435,15 @@ public MetaDataMutationResult validateAndAddMetadata(PTable table, byte[][] rowK | |||
} | |||
|
|||
private MetaDataMutationResult compareWithPkColumns(byte[] colName, | |||
PTable table, byte[] familyName) { | |||
PTable table, byte[] familyName) { |
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:
This looks worse than before
public static boolean isMaxLookbackTimeEnabled(Configuration conf){ | ||
return isMaxLookbackTimeEnabled(conf.getLong(PHOENIX_MAX_LOOKBACK_AGE_CONF_KEY, |
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 is not a scan attribute.
It may be better to move this to QueryServicesOptions.
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.
I think this class (and also the function) is moved to coprocessors package
import org.apache.phoenix.hbase.index.util.ImmutableBytesPtr; | ||
import org.apache.phoenix.memory.MemoryManager; | ||
|
||
import org.apache.phoenix.scanattributes.ServerCachingProtocol.ServerCacheFactory; |
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.
ditto
import org.apache.phoenix.hbase.index.util.IndexManagementUtil; | ||
import org.apache.phoenix.jdbc.PhoenixDatabaseMetaData; | ||
import org.apache.phoenix.query.QueryConstants; | ||
import org.apache.phoenix.query.QueryServices; | ||
import org.apache.phoenix.scanattributes.MetaDataProtocol; |
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.
change not needed
@@ -64,6 +62,7 @@ | |||
import org.apache.phoenix.jdbc.PhoenixConnection; | |||
import org.apache.phoenix.query.ConnectionQueryServices; | |||
import org.apache.phoenix.query.KeyRange; | |||
import org.apache.phoenix.scanattributes.MetaDataProtocol; |
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.
change not needed
import static org.apache.phoenix.jdbc.PhoenixDatabaseMetaData.VIEW_INDEX_ID_BYTES; | ||
import static org.apache.phoenix.jdbc.PhoenixDatabaseMetaData.VIEW_INDEX_ID_DATA_TYPE_BYTES; | ||
import static org.apache.phoenix.query.QueryConstants.DEFAULT_COLUMN_FAMILY_BYTES; | ||
import static org.apache.phoenix.scanattributes.MetaDataProtocol.MIN_SPLITTABLE_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.
change not needed
import org.apache.hadoop.hbase.client.TableDescriptorBuilder; | ||
import org.apache.phoenix.query.QueryConstants; | ||
|
||
public class IndexerStatic { |
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.
Move to IndexUtil or rename to IndexerUtil ?
c4d3811
to
a9c41e1
Compare
This looks OK to me. |
Can you rebase to the current master (unfortunately a few more rebases will likely be needed while the discussion is ongoing) ? |
I see you haven't touched phoenix-client. |
Unfortunately the Zookeeperless code made another large-ish rebase necessary. |
7f95ac4
to
40c2c20
Compare
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 change is huge, I tried to check it reasonably thorough but there might be things that I missed.
The change looks mostly good I only left some minor comments on it.
phoenix-tests/pom.xml
Outdated
</plugin> | ||
<plugin> | ||
<groupId>org.apache.maven.plugins</groupId> | ||
<artifactId>maven-jar-plugin</artifactId> |
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.
I see that it was moved from the phoenix-core, correctly
Lines 124 to 154 in 5128ad0
<plugin> | |
<groupId>org.apache.maven.plugins</groupId> | |
<artifactId>maven-jar-plugin</artifactId> | |
<executions> | |
<execution> | |
<phase>prepare-package | |
</phase> | |
<goals> | |
<goal>test-jar</goal> | |
</goals> | |
<configuration> | |
<archive> | |
<manifest> | |
<mainClass>org.apache.phoenix.util.GeneratePerformanceData</mainClass> | |
</manifest> | |
</archive> | |
</configuration> | |
</execution> | |
</executions> | |
<configuration> | |
<!-- Exclude these 2 packages, because their dependency _binary_ files | |
include the sources, and Maven 2.2 appears to add them to the sources to compile, | |
weird --> | |
<excludes> | |
<exclude>org/apache/jute/**</exclude> | |
<exclude>org/apache/zookeeper/**</exclude> | |
<exclude>**/*.jsp</exclude> | |
<exclude>log4j.properties</exclude> | |
</excludes> | |
</configuration> | |
</plugin> |
But I am not sure if we need this maven-jar-plugin, we could check if it can be removed
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.
I'm not sure either, but I think this should be investigated in a separate issue.
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.
I agree that we should not do it as part of this ticket.
phoenix-coprocessors/pom.xml
Outdated
</dependency> | ||
<dependency> | ||
<groupId>com.github.stephenc.findbugs</groupId> | ||
<artifactId>findbugs-annotations</artifactId> |
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 dependency is duplicated it is there in line 128 too
phoenix-coprocessors/pom.xml
Outdated
</dependency> | ||
|
||
|
||
<!-- Other dependencies --> |
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.
The order of these dependencies are different than the it was originally in the phoenix-core, making it hard to compare.
@@ -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; } |
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.
Why can't we keep this function in IndexUtil?
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.
I think Store is defined in hbase-server package, IndexUtil is in phoenix-core. We want to break (phoenix-core -> hbase-server) dependency so this function needs to be moved. As its only used in this file, which is indeed in pheonix-coprocessors (which can use hbase-server), I moved it here.
indexMaintainer.deleteRowIfAgedEnough(indexRowKey, indexRow.get(0).getTimestamp(), | ||
ageThreshold, false, region); | ||
if (indexMaintainer.isAgedEnough(ts, ageThreshold)) { | ||
region.delete(indexMaintainer.createDelete(indexRowKey, ts, false)); |
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.
What are the differences between region.delete and region.batchMutate(mutations); ?
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.
According to hbase documentation:
batchMutate: Perform a batch of mutations. Note this supports only Put and Delete mutations and will ignore other types passed.
delete: Deletes the specified cells/row.
In this specific case the batch contained only 1 delete, so changing it to a single delete operation should do the same.
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.
yep, that is right, that should do the same.
I was curious if you know if there is some performance difference between the 2.
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.
I suppose that possibly could differ between implementations, but HRegion just calls batchMutate in delete if I'm not mistaken.
@@ -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"; |
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.
Please use underscore to make these 4 variable more readable
} catch (SQLException e) { | ||
throw new IOException(e); | ||
} | ||
|
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.
I am not happy that the order of these function changed.
+ PhoenixRuntime.JDBC_PROTOCOL_SEPARATOR + zkClientPort | ||
+ PhoenixRuntime.JDBC_PROTOCOL_SEPARATOR + zkParentNode; | ||
} | ||
//TODO |
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.
Did it got copied to somewhere or what is pending here?
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.
no idea, we should leave this function alone.
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.
Actually, I think I know what happened here.
This function depends on hbase-server which we don't want in phoenix-core. So it should be moved somewhere else, but this fn is not used anywhere. So probably we could just delete it. What do you think?
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.
Dead functions can be removed.
@@ -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 |
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.
We can write this to one line
@@ -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)); |
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 is bit weird calling ClientUtil in ServerUtilTest
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.
Phoenix server side is a superset of client side.
But this could be split to a separate test class.
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.
Fortunately this test class only contains tests for ClientUtil, so I will just rename it to ClientUtilTest.
79c6f0e
to
00ff1da
Compare
@richardantal updated the PR based on your comments. |
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.
Thank you for doing this change.
I really appreciate the effort that went into this one.
Keep up the fantastic work.
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.
Phoenix-server is broken.
You need to add the phoenix-coprocessors dependency (or rather replace the phoenix-core dependency with phoenix-coprocessors which should pull in core transitively)
Edit: not needed, do as you prefer: I suggested moving BaseIndexCodec one package higher, but now I see that it was unneccessary. |
ServerIndexUtil and ServerViewUtil get added to phoenix-client-embedded. |
ServerIndexUtil and ServerViewUtil get added to phoenix-client-embedded because we depend on phoenix-coprocessors for compatibility reasons. So that's fine. |
I have also renamed the ticket, so please update the commit message to the new name as well. |
@stoty Added phoenix-coprocessors dependency to phoenix-server and modified the commit msg. |
I got this error when running mvn install. [INFO] --- maven-dependency-plugin:3.1.1:analyze-only (enforce-dependencies) @ phoenix-tests --- Test dependencies are not transitive, you need to add everything that phoenix-core depends on. |
Come to think of it, we should do this properly. |
Ignore my previous comment, I was wrong. |
The dependency-plugin will warn about all undefined dependencies, it explicitly doesn't want to see transitive dependencies. |
We could in fact change the scope to test for most dependencies. |
You only need to add the zookeeper-jute dependency |
Weird, if I add zookeeper-jute I get the following error: [INFO] --- dependency:3.1.1:analyze-only (enforce-dependencies) @ phoenix-tests --- |
It depends on the java version. But if we add it, then Java 8 fails with "Unused declared dependencies found:" Please add the exclusion both ways into the maven-dependency-plugin config, with a comment explaining the situation. |
Do we need to add two way exclusion? |
That's also fine. I have disassembled tests, and even though there is no include for zookeeper-jute, it does in fact call a method whoose return value is defined in zk-jute. Why this is dependent on java version is beyond me. |
@stoty I've added the zk-jute dependency and also the exclusion. |
Thank you, @Aarchy . |
As discussed ofline, we have found a more compatible module naming scheme: phoenix-core depends on phoenix-core-server and phoenix-server-client, and includes all the tests. This maximizes compatbility with existing maven consumers and downstream projects. |
We can put phoenix-core-client and phoenix-core-server under the phoenix-core directory, or we can have a flat directory structure, I don't really have a preference. |
You also need adopt (or rather revert) phoenix-sandbox.py to these changes. |
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 LGTM
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 I looked into the the Apache CI tests,and every single one has timed out.
Need to dig into that before merging.
WALRecoveryRegionPostOpenIT is hanging (at least in the latest run) |
The hang is definielty caused by this patch. |
So I have confirmed that the hang is caused by this patch. The test ran fine more than five times in a row without the patch, but failed every time I tried with the patch. It doesn't seem to be a problem with the test code itself, so the patch must have introduced some other error, which causes the hang. Now we just need to figure out which one of the 1300 changed files went wrong. |
@stoty Thanks for pointing this out and the offline help as well. |
No description provided.