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

TableAdapter & TableAdapter2x Vaneer adaption #2006

Merged
merged 5 commits into from
Nov 22, 2018
Merged

TableAdapter & TableAdapter2x Vaneer adaption #2006

merged 5 commits into from
Nov 22, 2018

Conversation

rahulKQL
Copy link
Contributor

This PR containes changes related to:

  • Updated TableAdapter & TableAdapter2x to adapt to v2.models.CreateTableRequest.
  • updated ColumnDescriptorAdapter#buildGarbageCollectionRule to adapt to v2.models.GCRules.GCRule.
  • hbase1_x.BigtableAdmin, hbase2_x.BigtableAdmin & AbstractBigtableAdmin contains toProto() & TOODs, which will be removed with Admin method changes  #1985.

@rahulKQL rahulKQL requested a review from sduskis November 21, 2018 09:02
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Nov 21, 2018
@rahulKQL rahulKQL self-assigned this Nov 21, 2018
@sduskis
Copy link
Contributor

sduskis commented Nov 21, 2018

We should tag all adapters with an InternalAPI annotation, if one exists in GAX.

@rahulKQL
Copy link
Contributor Author

  • removed TableAdapter2.x#adapt(TableDescripter) & TableAdapter2x#toColumnFamily as it was not being used.
  • added @InternalApi annotations on TableAdapter class.

sduskis
sduskis suggested changes Nov 21, 2018
View reviewed changes
Copy link
Contributor

@sduskis sduskis left a comment

Choose a reason for hiding this comment

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

I have a few small changes I'd like to see, but otherwise things look great.

...igtable-hbase/src/main/java/com/google/cloud/bigtable/hbase/adapters/admin/TableAdapter.java Outdated Show resolved Hide resolved
...igtable-hbase/src/main/java/com/google/cloud/bigtable/hbase/adapters/admin/TableAdapter.java Show resolved Hide resolved
...igtable-hbase/src/main/java/com/google/cloud/bigtable/hbase/adapters/admin/TableAdapter.java Outdated Show resolved Hide resolved
...igtable-hbase/src/main/java/com/google/cloud/bigtable/hbase/adapters/admin/TableAdapter.java Outdated Show resolved Hide resolved
...arent/bigtable-hbase/src/main/java/org/apache/hadoop/hbase/client/AbstractBigtableAdmin.java Outdated
@@ -121,6 +123,7 @@ public AbstractBigtableAdmin(CommonConnection connection) throws IOException {
disabledTables = connection.getDisabledTables();
bigtableInstanceName = options.getInstanceName();
tableAdapter = new TableAdapter(bigtableInstanceName);
this.instanceName = InstanceName.of(bigtableInstanceName.getProjectId(), bigtableInstanceName.getInstanceId());
Copy link
Contributor

Choose a reason for hiding this comment

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

Please get the instance name from bigtableInstanceName.toGcbInstanceName(), if it exists.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Apparently v2.models.CreateTableRequest expects com.gogle.bigtable.admin.v2.InstanceName.
and bigtableInstanceName.toGcbInstanceName() returns v2.models.InstanceName.

Copy link
Contributor

Choose a reason for hiding this comment

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

ugh. Please add a bigtableInstanceName.toAdminInstanceName()

Copy link
Collaborator

@igorbernstein2 igorbernstein2 Nov 21, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Please note that I will be removing Names from the google-cloud-java to avoid these kind of headaches.
What is toGcbInstanceName?

ref: googleapis/google-cloud-java#4091

Copy link
Contributor

Choose a reason for hiding this comment

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

@igorbernstein2, We have a BigtableInstanceName in this repo. toGcbInstanceName converts the BigtableInstanceName to an InstanceName. See here.

...arent/bigtable-hbase/src/main/java/org/apache/hadoop/hbase/client/AbstractBigtableAdmin.java Outdated Show resolved Hide resolved
...arent/bigtable-hbase/src/main/java/org/apache/hadoop/hbase/client/AbstractBigtableAdmin.java Outdated Show resolved Hide resolved
...arent/bigtable-hbase/src/main/java/org/apache/hadoop/hbase/client/AbstractBigtableAdmin.java Outdated Show resolved Hide resolved
.../bigtable-hbase-2.x/src/main/java/com/google/cloud/bigtable/hbase2_x/BigtableAsyncAdmin.java Outdated Show resolved Hide resolved
...base-2.x/src/main/java/com/google/cloud/bigtable/hbase2_x/adapters/admin/TableAdapter2x.java Outdated Show resolved Hide resolved
@sduskis
Copy link
Contributor

sduskis commented Nov 21, 2018

@rahulKQL, it looks like the GCRule object has issues: googleapis/google-cloud-java#4057. Please add unit tests to TableAdapter.

igorbernstein2
igorbernstein2 reviewed Nov 21, 2018
View reviewed changes
...se/src/main/java/com/google/cloud/bigtable/hbase/adapters/admin/ColumnDescriptorAdapter.java Show resolved Hide resolved
...se/src/main/java/com/google/cloud/bigtable/hbase/adapters/admin/ColumnDescriptorAdapter.java Outdated Show resolved Hide resolved
...igtable-hbase/src/main/java/com/google/cloud/bigtable/hbase/adapters/admin/TableAdapter.java Outdated Show resolved Hide resolved
...arent/bigtable-hbase/src/main/java/org/apache/hadoop/hbase/client/AbstractBigtableAdmin.java Outdated
@@ -121,6 +123,7 @@ public AbstractBigtableAdmin(CommonConnection connection) throws IOException {
disabledTables = connection.getDisabledTables();
bigtableInstanceName = options.getInstanceName();
tableAdapter = new TableAdapter(bigtableInstanceName);
this.instanceName = InstanceName.of(bigtableInstanceName.getProjectId(), bigtableInstanceName.getInstanceId());
Copy link
Collaborator

@igorbernstein2 igorbernstein2 Nov 21, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Please note that I will be removing Names from the google-cloud-java to avoid these kind of headaches.
What is toGcbInstanceName?

ref: googleapis/google-cloud-java#4091

@rahulKQL
Copy link
Contributor Author

  • Added unit tests TestTableAdapter & TestTableAdapter2x.
  • Removed unused methods from ColumnDescriptorAdapter.
  • Changes for variable names & arguments

@sduskis @igorbernstein2
Could you please review it again.

@sduskis sduskis merged commit b78bb15 into googleapis:master Nov 22, 2018
rahulKQL added a commit to rahulKQL/java-bigtable-hbase that referenced this pull request Nov 29, 2018
@rahulKQL
TableAdapter & TableAdapter2x Vaneer adaption (googleapis#2006)
a15fdc0 
* adding TableAdapter and related changes

* fixed typo error on InstanceId

* removed unnecessart TODOs & reverted toColumnFamily

* removed unused TableAdapter#adapt and added @internalapi

* added Unit Tests, refactored varible name & unused methods
@rahulKQL rahulKQL deleted the tableAdapter_changes branch December 3, 2018 07:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@igorbernstein2 igorbernstein2 igorbernstein2 left review comments

@sduskis sduskis sduskis approved these changes

Assignees

@rahulKQL rahulKQL

Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@rahulKQL @sduskis @igorbernstein2 @googlebot