Skip to content
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-4906 Introduce a master coprocessor to handle cases where merging regions of salted table might cause an issue #1552

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

mnpoonia
Copy link
Contributor

No description provided.

@stoty
Copy link
Contributor

stoty commented Jan 31, 2023

I suggest using a more generic name, like PhoenixMasterObserver.

This will make our lives easier if we ever need to add more features to it.

@mnpoonia mnpoonia marked this pull request as ready for review March 3, 2023 08:22
@mnpoonia
Copy link
Contributor Author

mnpoonia commented Mar 3, 2023

@stoty i have changed the names accordingly. Please review when you get some cycles.

@mnpoonia mnpoonia force-pushed the salt_sp_cases branch 2 times, most recently from 56edc6c to fa00f5b Compare March 3, 2023 08:52
…ging regions of salted table might cause an issue
Configuration conf = ctx.getEnvironment().getConfiguration();
PreparedStatement stmt;
ResultSet resultSet;
try (Connection conn = QueryUtil.getConnectionOnServer(conf)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we should get the salted status via a PTable object from the CQSI cache instead.
Wouldn't that be faster ?

@BeforeClass
public static synchronized void doSetup() throws Exception {
Map<String, String> serverProps = Maps.newHashMapWithExpectedSize(3);
serverProps.put("hbase.coprocessor.master.classes", PhoenixMasterObserver.class.getName());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no way to automatically load a Master coprocessor on Phoenix startup, right ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think better to use config for master/regionserver coproc loading. Even if we do figure out letting phoenix update coproc at runtime, pretty sure we would end up using private/unstable APIs that can break across releases.

I was wondering this same point sometime back and hence decided to document this detail that we need to use this coproc by updating hbase config, having no other choice but to document it #1561 (comment)

@stoty
Copy link
Contributor

stoty commented Mar 3, 2023

Also, since this is a broad topic, perhaps we could split this feature into its separate JIRA ?

@mnpoonia
Copy link
Contributor Author

mnpoonia commented Mar 7, 2023

I am mostly positive about this change but need help in two things

  • How can we check if a table is a phoenix table or not?
  • How do we want to add this coprocessor to system? Through hbase-site config(hbase.coprocessor.master.classes) or through phoenix code (i am not aware of this path) ?

@virajjasani @kadirozde @apurtell @stoty Looking forward for your suggestions

@stoty
Copy link
Contributor

stoty commented Mar 7, 2023

I think using something like org.apache.phoenix.util.PhoenixRuntime.getTable(Connection, String)is the easiest (and perhaps fastest).
PTable also tells us if the table is salted.

As for the processor, I could not find a way to load from code, it has to be configured manually.
Phoenix already has a similar requirement for the old indexing's WAL Codec, so while not ideal, it's not a problem.

@jpisaac
Copy link
Contributor

jpisaac commented Mar 7, 2023

Few questions @mnpoonia

  1. Shouldn't this be handled in HBase purely instead of involving Phoenix, as I understand salting is an HBase feature?

A couple of questions if Phoenix needs to be involved
2. Shouldn't there be some checks if the Phoenix subsystem is present or not?
3. What are the implications on dependency management (since there is a tight coupling here between HBase master services and phoenix) during upgrades and maintenance?

@virajjasani @apurtell your thoughts?

@jpisaac
Copy link
Contributor

jpisaac commented Mar 7, 2023

@mnpoonia Also can we ensure that it is not a Phoenix Bug in ScanRanges?

@stoty
Copy link
Contributor

stoty commented Mar 7, 2023

Salting is not an HBase feature, it's a Phoenix feature (at least as used by Phoenix).

Ideally, HBase had a MergePolicy mechanism simliar to the SplitPolicy mechanism where we could add this logic per table, but it doesn't.
Even if we were to add it, it wouldn't be available before 2.6 or even later.

I admit I that I have never really tracked down why we have this merging restriction, but it seems to be a performance optimization.

@virajjasani virajjasani self-requested a review March 7, 2023 20:19
@jpisaac
Copy link
Contributor

jpisaac commented Mar 8, 2023

Salting is not an HBase feature, it's a Phoenix feature (at least as used by Phoenix).

@stoty I referring to what was explained here in the hot-spotting section of the hbase book. I think Phoenix just facilitates this under the cover for the user.

What I was asking is shouldn't we be seeing this issue in a purely HBase table with row keys distributed with salt prefixes?

I also think we should investigate more why the scanRanges are behaving such. Will try and dig more.

@stoty
Copy link
Contributor

stoty commented Mar 8, 2023

Yes, the problem is caused by this assumption built into the Phoenix implementation.
If we could remove that without hurting performance, that would be brilliant.
Thank you for digging into it.

@mnpoonia
Copy link
Contributor Author

mnpoonia commented Mar 9, 2023

@stoty I referring to what was explained here in the hot-spotting section of the hbase book. I think Phoenix just facilitates this under the cover for the user.

What I was asking is shouldn't we be seeing this issue in a purely HBase table with row keys distributed with salt prefixes?

@jpisaac I am not sure but my understanding is that hbase doesnot have a concept of salting. Clients can use salting to reduce the hotpostitng but then it is upto clients that how they want to manage all aspects of it. HBase doesn't care about salting in case of split or merge.

In phoenix we have this limitation that every row in a single region should have same salt. If we mix(merge) two regions of different salts phoenix scans starting behaving weird.

if(saltBuckets > 0 ) {
System.out.println("Number of buckets="+saltBuckets);
return !regionsHaveSameSalt(regionsToMerge);
if(saltBuckets <= 0 ) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Having negative salt buckets here points to some serious problem with the tabke.
I'd just check for 0 here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants