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.

Sign up for GitHub

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

Jump to bottom

feat: add HBase SDK for serving #127

Merged
merged 33 commits into from
Oct 2, 2024
Merged

feat: add HBase SDK for serving #127

merged 33 commits into from
Oct 2, 2024

Conversation

bayu-aditya
Copy link
Collaborator

@bayu-aditya bayu-aditya commented Sep 3, 2024 •
edited by shydefoo
Loading

Summary

This PR adds support to use hbase as the online store.

  • Read data from BigTable and Hbase in caraml-serving by using HBase SDK by set configuration caraml.store.bigtable.isUsingHBaseSDK become true. If the value is false, then it will read data by using BigTable SDK.
  • Modifies spark job to ingest the data into HBase for stream and batch ingestion, but reuses most of the existing logic to pull data from BigTable.
  • Add additional unit tests to read data from bigtable using HBase sdk, and to read data from hbase directly
  • Refactor bigtable online store to share BaseSchemaRegistry

@bayu-aditya bayu-aditya changed the title Retrieve HBase data from local feat: add HBase SDK for serving Sep 4, 2024
@bayu-aditya bayu-aditya force-pushed the bayu/hbase branch 2 times, most recently from 4dc8df6 to fd81384 Compare September 4, 2024 08:17
@shydefoo
Add spark changes to use hbase api
ad454e1
@shydefoo
Fix connection issues
2528ffe
@shydefoo
Fix linting
773eac1
@shydefoo
Add configuration for hbase api
fd3030a
@shydefoo
Set platform to linux/amd64
cdfad3d
@shydefoo
Update application.yaml to include hbase
73dfd8e
@shydefoo
Refator BigTableSinkRelation to use updated classes
acd54c2 
Use of deprecated classes result in ImmutableHTableDescriptor returned which throws java.lang.UnsupportedOperationException: HTableDescriptor is read-only error
@shydefoo
Fix issue due to difference in bigtable and hbase response
f685315 
* Use offset and length to get rowCell values because hbase server
  returns slightly different response structure than bigtable
* This is also applied when looking up the avro schema
@shydefoo
Fix linting
0054a14
@shydefoo shydefoo mentioned this pull request Sep 9, 2024
Merged
shydefoo and others added 9 commits September 9, 2024 11:37
@shydefoo
Fix issue due to difference in bigtable and hbase response
e1840ef 
* Use offset and length to get rowCell values because hbase server
  returns slightly different response structure than bigtable
* This is also applied when looking up the avro schema
@shydefoo
Fix linting
0eb1b13
@shydefoo
Remove commented code
350783e
@shydefoo
Clean up comments
38f7bc7
@shydefoo
Fix application yaml
691c8d1
@shydefoo
Merge branch 'bayu/hbase' into hbase-poc
8855d0a
@shydefoo
Add option for hbase for stream ingestion jobs
a0e44a3
@shydefoo
Fix linting
fa28a59
@shydefoo
Merge pull request #126 from caraml-dev/hbase-poc
7292b48 
[feat] Add support to use hbase online store
@bayu-aditya bayu-aditya marked this pull request as ready for review September 20, 2024 02:31
@bayu-aditya bayu-aditya requested review from tiopramayudi and removed request for tiopramayudi, bthari and deadlycoconuts September 23, 2024 06:27
@tiopramayudi
Copy link

can you update the PR description and highlight what are the primary changes?

tiopramayudi
tiopramayudi reviewed Sep 24, 2024
View reviewed changes
caraml-store-serving/src/main/java/dev/caraml/serving/store/bigtable/BigTableStoreConfig.java Outdated

@Bean
public OnlineRetriever getRetriever() {
// Using HBase SDK
if (isUsingHBaseSDK) {
org.apache.hadoop.conf.Configuration config =
Copy link

Choose a reason for hiding this comment

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

should this block wrapped by try catch block? I saw that the connect method can throw the IllegalStateException

Copy link
Contributor

@shydefoo shydefoo Sep 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@bayu-aditya could you take a look at this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yup sure

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done in here 455e253 Thank you for your finding

Copy link

Choose a reason for hiding this comment

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

should we use one try catch and condition is in the try catch block?

caraml-store-serving/src/main/java/dev/caraml/serving/store/bigtable/HBaseOnlineRetriever.java Outdated Show resolved Hide resolved
caraml-store-serving/src/main/java/dev/caraml/serving/store/bigtable/HBaseSchemaRegistry.java Outdated
@Override
public int hashCode() {
int result = tableName.hashCode();
result = 31 * result + schemaHash.hashCode();
Copy link

Choose a reason for hiding this comment

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

what is 31 represent for?

Copy link
Contributor

Choose a reason for hiding this comment

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

I have no idea 😆

caraml-store-serving/src/main/java/dev/caraml/serving/store/bigtable/HBaseSchemaRegistry.java Outdated Show resolved Hide resolved
caraml-store-spark/docker/Dockerfile Show resolved Hide resolved
@shydefoo shydefoo requested review from tiopramayudi and removed request for shydefoo September 26, 2024 10:26
@shydefoo
Copy link
Contributor

@tiopramayudi @mbruner poke

tiopramayudi
tiopramayudi approved these changes Oct 1, 2024
View reviewed changes
Copy link

@tiopramayudi tiopramayudi left a comment

Choose a reason for hiding this comment

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

Add two more feedbacks, the rest is LGTM. Thanks @bayu-aditya @shydefoo

caraml-store-serving/src/main/java/dev/caraml/serving/store/bigtable/HBaseOnlineRetriever.java Outdated
return Collections.<Feature>emptyList();
} else {
Result row = rows.get(rowKey);
return featureReferences.stream()
Copy link

Choose a reason for hiding this comment

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

the chain is very long, is it possible to break the chain?

Copy link
Collaborator Author

@bayu-aditya bayu-aditya Oct 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

done in 4355a1a thank you 🙏

caraml-store-serving/src/main/java/dev/caraml/serving/store/bigtable/BigTableStoreConfig.java Outdated

@Bean
public OnlineRetriever getRetriever() {
// Using HBase SDK
if (isUsingHBaseSDK) {
org.apache.hadoop.conf.Configuration config =
Copy link

Choose a reason for hiding this comment

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

should we use one try catch and condition is in the try catch block?

@bayu-aditya bayu-aditya merged commit b343616 into main Oct 2, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shydefoo shydefoo shydefoo left review comments

@tiopramayudi tiopramayudi tiopramayudi approved these changes

@mbruner mbruner Awaiting requested review from mbruner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@bayu-aditya @tiopramayudi @shydefoo