Skip to content

Commit b28ce92

Browse files
spollapallyspollapally
committed
code review updates.
1 parent 05d381b commit b28ce92

File tree

9 files changed

+272
-191
lines changed

9 files changed

+272
-191
lines changed

‎google-cloud-clients/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/admin/v2/TableAdminClient.java‎

Lines changed: 95 additions & 97 deletions
Large diffs are not rendered by default.

‎google-cloud-clients/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/admin/v2/models/GCRules.java‎

Lines changed: 28 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -71,9 +71,7 @@ public VersionRule maxVersions(int maxVersion) {
7171
* @return DurationRule
7272
*/
7373
public DurationRule maxAge(long maxAge, TimeUnit timeUnit) {
74-
Duration duration = Duration.ZERO;
75-
TimeUnit.SECONDS.convert(maxAge, timeUnit);
76-
return maxAge(duration);
74+
return maxAge(Duration.ofNanos(TimeUnit.NANOSECONDS.convert(maxAge, timeUnit)));
7775
}
7876

7977
/**
@@ -100,11 +98,10 @@ public DefaultRule defaulRule() {
10098
* intersection as the root
10199
*/
102100
public static final class IntersectionRule extends BaseRule {
103-
private GcRule.Intersection.Builder builder;
104-
private List<GCRule> rulesList = new ArrayList<>();
101+
private final List<GCRule> rulesList;
105102

106103
private IntersectionRule() {
107-
this.builder = GcRule.Intersection.newBuilder();
104+
rulesList = new ArrayList<>();
108105
}
109106

110107
/**
@@ -115,7 +112,6 @@ private IntersectionRule() {
115112
*/
116113
public IntersectionRule rule(@Nonnull GCRule rule) {
117114
rulesList.add(rule);
118-
builder.addRules(rule.toProto());
119115
return this;
120116
}
121117

@@ -136,13 +132,16 @@ public String toString() {
136132
@InternalApi
137133
@Override
138134
public GcRule toProto() {
139-
switch (builder.getRulesCount()) {
135+
switch (rulesList.size()) {
140136
case 0:
141137
return GcRule.newBuilder().build();
142138
case 1:
143-
return builder.getRules(0);
139+
return rulesList.get(0).toProto();
144140
default:
145-
return GcRule.newBuilder().setIntersection(builder.build()).build();
141+
return GcRule.newBuilder()
142+
.setIntersection(
143+
Intersection.newBuilder().addAllRules(convertToGcRules(rulesList)))
144+
.build();
146145
}
147146
}
148147
}
@@ -152,11 +151,10 @@ public GcRule toProto() {
152151
* the root
153152
*/
154153
public static final class UnionRule extends BaseRule {
155-
private GcRule.Union.Builder builder;
156-
private List<GCRule> rulesList = new ArrayList<>();
154+
private final List<GCRule> rulesList;
157155

158156
private UnionRule() {
159-
this.builder = GcRule.Union.newBuilder();
157+
rulesList = new ArrayList<>();
160158
}
161159

162160
/**
@@ -167,7 +165,6 @@ private UnionRule() {
167165
*/
168166
public UnionRule rule(@Nonnull GCRule rule) {
169167
rulesList.add(rule);
170-
builder.addRules(rule.toProto());
171168
return this;
172169
}
173170

@@ -188,20 +185,23 @@ public String toString() {
188185
@InternalApi
189186
@Override
190187
public GcRule toProto() {
191-
switch (builder.getRulesCount()) {
188+
switch (rulesList.size()) {
192189
case 0:
193190
return GcRule.newBuilder().build();
194191
case 1:
195-
return builder.getRules(0);
192+
return rulesList.get(0).toProto();
196193
default:
197-
return GcRule.newBuilder().setUnion(builder.build()).build();
194+
return GcRule.newBuilder()
195+
.setUnion(
196+
Union.newBuilder().addAllRules(convertToGcRules(rulesList)))
197+
.build();
198198
}
199199
}
200200
}
201201

202202
/** Wrapper for building max versions rule */
203203
public static final class VersionRule extends BaseRule {
204-
private GcRule.Builder builder;
204+
private final GcRule.Builder builder;
205205

206206
private VersionRule(int maxVersion) {
207207
this.builder = GcRule.newBuilder();
@@ -227,7 +227,7 @@ public GcRule toProto() {
227227

228228
/** Wrapper for building max duration rule */
229229
public static final class DurationRule extends BaseRule {
230-
private com.google.protobuf.Duration.Builder builder;
230+
private final com.google.protobuf.Duration.Builder builder;
231231

232232
private DurationRule(Duration duration) {
233233
this.builder =
@@ -312,4 +312,13 @@ public interface GCRule {
312312
@InternalApi
313313
GcRule toProto();
314314
}
315+
316+
private static List<GcRule> convertToGcRules(List<GCRule> rules) {
317+
List<GcRule> gcRules = new ArrayList<>(rules.size());
318+
319+
for (GCRule rule : rules) {
320+
gcRules.add(rule.toProto());
321+
}
322+
return gcRules;
323+
}
315324
}

‎google-cloud-clients/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/admin/v2/models/TableAdminRequests.java‎

Lines changed: 21 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -19,14 +19,13 @@
1919
import com.google.api.core.InternalApi;
2020
import com.google.bigtable.admin.v2.ColumnFamily;
2121
import com.google.bigtable.admin.v2.CreateTableRequest;
22-
import com.google.bigtable.admin.v2.Table;
23-
import com.google.bigtable.admin.v2.TableName;
24-
import com.google.bigtable.admin.v2.Table.TimestampGranularity;
2522
import com.google.bigtable.admin.v2.CreateTableRequest.Split;
26-
import com.google.bigtable.admin.v2.GcRule;
2723
import com.google.bigtable.admin.v2.InstanceName;
2824
import com.google.bigtable.admin.v2.ModifyColumnFamiliesRequest;
2925
import com.google.bigtable.admin.v2.ModifyColumnFamiliesRequest.Modification;
26+
import com.google.bigtable.admin.v2.Table;
27+
import com.google.bigtable.admin.v2.Table.TimestampGranularity;
28+
import com.google.bigtable.admin.v2.TableName;
3029
import com.google.cloud.bigtable.admin.v2.models.GCRules.GCRule;
3130
import com.google.common.base.Preconditions;
3231
import com.google.protobuf.ByteString;
@@ -59,12 +58,12 @@ public static ModifyFamilies modifyFamilies(String tableId) {
5958
/**
6059
* Fluent wrapper for {@link CreateTableRequest}
6160
*
62-
* <pre>
63-
* Allows for creating table with
64-
* - optional columnFamilies, including optional {@link GCRule}
65-
* - optional granularity
66-
* - and optional split points
67-
* </pre>
61+
* <p> Allows for creating table with:
62+
* <ul>
63+
* <li> optional columnFamilies, including optional {@link GCRule}
64+
* <li> optional granularity
65+
* <li> and optional split points
66+
* </ul>
6867
*/
6968
public static final class CreateTable {
7069
private final CreateTableRequest.Builder createTableRequest = CreateTableRequest.newBuilder();
@@ -150,17 +149,17 @@ public CreateTableRequest toProto(InstanceName instanceName) {
150149
/**
151150
* Fluent wrapper for {@link ModifyColumnFamiliesRequest}
152151
*
153-
* <pre>
154-
* Allows the following ColumnFamily modifications
155-
* - create family, optionally with {@link GCRule}
156-
* - update existing family {@link GCRule}
157-
* - drop an existing family
158-
* </pre>
152+
* <p> Allows for the following ColumnFamily modifications:
153+
* <ul>
154+
* <li> create family, optionally with {@link GCRule}
155+
* <li> update existing family {@link GCRule}
156+
* <li> drop an existing family
157+
* </ul>
159158
*/
160159
public static final class ModifyFamilies {
161160
private final ModifyColumnFamiliesRequest.Builder modFamilyRequest =
162161
ModifyColumnFamiliesRequest.newBuilder();
163-
private String tableId;
162+
private final String tableId;
164163

165164
/**
166165
* Configures the tableId to execute the modifications
@@ -179,7 +178,7 @@ private ModifyFamilies(String tableId) {
179178
* @return
180179
*/
181180
public ModifyFamilies create(String familyId) {
182-
return createWithGCRule(familyId, null);
181+
return create(familyId, GCRules.GCRULES.defaulRule());
183182
}
184183

185184
/**
@@ -189,10 +188,10 @@ public ModifyFamilies create(String familyId) {
189188
* @param gcRule
190189
* @return
191190
*/
192-
public ModifyFamilies createWithGCRule(String familyId, GCRule gcRule) {
191+
public ModifyFamilies create(String familyId, GCRule gcRule) {
193192
Modification.Builder modification = Modification.newBuilder().setId(familyId);
194-
GcRule grule = (gcRule == null) ? GcRule.getDefaultInstance() : gcRule.toProto();
195-
modification.setCreate(ColumnFamily.newBuilder().setGcRule(grule));
193+
Preconditions.checkNotNull(gcRule);
194+
modification.setCreate(ColumnFamily.newBuilder().setGcRule(gcRule.toProto()));
196195
modFamilyRequest.addModifications(modification.build());
197196
return this;
198197
}
@@ -204,7 +203,7 @@ public ModifyFamilies createWithGCRule(String familyId, GCRule gcRule) {
204203
* @param gcRule
205204
* @return
206205
*/
207-
public ModifyFamilies updateWithGCRule(String familyId, GCRule gcRule) {
206+
public ModifyFamilies update(String familyId, GCRule gcRule) {
208207
Modification.Builder modification = Modification.newBuilder().setId(familyId);
209208
Preconditions.checkNotNull(gcRule);
210209
modification.setUpdate(ColumnFamily.newBuilder().setGcRule(gcRule.toProto()));

‎google-cloud-clients/google-cloud-bigtable/src/main/java/com/google/cloud/bigtable/admin/v2/models/TableAdminResponses.java‎

Lines changed: 46 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,7 @@ private TableAdminResponses() {}
4646
/**
4747
* Converts the protocol buffer table to a simpler Table model with only the required elements
4848
*
49-
* @param com.google.bigtable.admin.v2.Table - Protobuf table
49+
* @param table - Protobuf table
5050
* @return Table - Table response wrapper
5151
*/
5252
@InternalApi
@@ -58,39 +58,62 @@ public static Table convertTable(com.google.bigtable.admin.v2.Table table) {
5858
* Converts the protocol buffer response to a simpler ConsistencyToken which can be passed back as
5959
* is.
6060
*
61-
* @param GenerateConsistencyTokenResponse - Protobuf ConsistencyTokenResponse
61+
* @param tokenResponse - Protobuf ConsistencyTokenResponse
6262
* @return ConsistencyToken - ConsistencyToken response wrapper
6363
*/
64+
@InternalApi
6465
public static ConsistencyToken convertTokenResponse(
6566
GenerateConsistencyTokenResponse tokenResponse) {
6667
return new ConsistencyToken(tokenResponse);
6768
}
6869

70+
/**
71+
* Converts the protocol buffer response to a simpler ClusterState model with only required elements
72+
*
73+
* @param clusterStatesMap - Protobuf clusterStatesMap
74+
* @return Map<String, ClusterState>
75+
*/
76+
@InternalApi
77+
public static Map<String, ClusterState> convertClusterStates(
78+
Map<String, com.google.bigtable.admin.v2.Table.ClusterState> clusterStatesMap) {
79+
Map<String, ClusterState> clusterStates = new HashMap<>();
80+
81+
for (Entry<String, com.google.bigtable.admin.v2.Table.ClusterState> entry : clusterStatesMap.entrySet()) {
82+
clusterStates.put(entry.getKey(), new ClusterState(entry.getKey(), entry.getValue()));
83+
}
84+
return clusterStates;
85+
}
86+
87+
/**
88+
* Converts the protocol buffer response to a simpler ColumnFamily model with only required elements
89+
*
90+
* @param columnFamiliesMap - Protobuf columnFamiliesMap
91+
* @return Map<String, ColumnFamily>
92+
*/
93+
@InternalApi
94+
public static Map<String, ColumnFamily> convertColumnFamilies(
95+
Map<String, com.google.bigtable.admin.v2.ColumnFamily> columnFamiliesMap) {
96+
Map<String, ColumnFamily> columnFamilies = new HashMap<>();
97+
98+
for (Entry<String, com.google.bigtable.admin.v2.ColumnFamily> entry : columnFamiliesMap.entrySet()) {
99+
columnFamilies.put(entry.getKey(), new ColumnFamily(entry.getKey(), entry.getValue()));
100+
}
101+
return columnFamilies;
102+
}
103+
69104
/** Wrapper for {@link Table} protocol buffer object */
70105
public static final class Table {
71-
private TableName tableName;
72-
private TimestampGranularity timestampGranularity;
73-
private Map<String, ClusterState> clusterStates = new HashMap<>();
74-
private Map<String, ColumnFamily> columnFamilies = new HashMap<>();
106+
private final TableName tableName;
107+
private final TimestampGranularity timestampGranularity;
108+
private final Map<String, ClusterState> clusterStates;
109+
private final Map<String, ColumnFamily> columnFamilies;
75110

76111
private Table(com.google.bigtable.admin.v2.Table table) {
77112
Preconditions.checkNotNull(table);
78113
this.tableName = TableName.parse(table.getName());
79114
this.timestampGranularity = table.getGranularity();
80-
81-
Map<String, com.google.bigtable.admin.v2.Table.ClusterState> clusterStatesMap =
82-
table.getClusterStatesMap();
83-
for (Entry<String, com.google.bigtable.admin.v2.Table.ClusterState> entry :
84-
clusterStatesMap.entrySet()) {
85-
clusterStates.put(entry.getKey(), new ClusterState(entry.getKey(), entry.getValue()));
86-
}
87-
88-
Map<String, com.google.bigtable.admin.v2.ColumnFamily> columnFamiliesMap =
89-
table.getColumnFamiliesMap();
90-
for (Entry<String, com.google.bigtable.admin.v2.ColumnFamily> entry :
91-
columnFamiliesMap.entrySet()) {
92-
columnFamilies.put(entry.getKey(), new ColumnFamily(entry.getKey(), entry.getValue()));
93-
}
115+
this.clusterStates = convertClusterStates(table.getClusterStatesMap());
116+
this.columnFamilies = convertColumnFamilies(table.getColumnFamiliesMap());
94117
}
95118

96119
/**
@@ -162,8 +185,8 @@ public String toString() {
162185

163186
/** Wrapper for {@link ClusterState} protocol buffer object */
164187
public static final class ClusterState {
165-
private String id;
166-
private ReplicationState replicationState;
188+
private final String id;
189+
private final ReplicationState replicationState;
167190

168191
private ClusterState(String id, com.google.bigtable.admin.v2.Table.ClusterState clusterState) {
169192
this.id = id;
@@ -280,7 +303,7 @@ private GCRule convertGcRule(GcRule source) {
280303
* They are obtained by invoking {@link TableAdminClient#generateConsistencyToken(String)}
281304
*/
282305
public static final class ConsistencyToken {
283-
private String token;
306+
private final String token;
284307

285308
private ConsistencyToken(GenerateConsistencyTokenResponse resp) {
286309
this.token = resp.getConsistencyToken();

‎google-cloud-clients/google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/admin/v2/TableAdminClientTest.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -212,7 +212,7 @@ public void getDropRowRangeRequest() {
212212
}
213213

214214
@Test
215-
public void getDropRowRangeRequest_dropAllData() {
215+
public void getDropRowRangeRequestDropAllData() {
216216
DropRowRangeRequest actual = adminClient.composeDropRowRangeRequest("tableId", null, true);
217217

218218
DropRowRangeRequest expected =

‎google-cloud-clients/google-cloud-bigtable/src/test/java/com/google/cloud/bigtable/admin/v2/it/TableAdminClientIT.java‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -41,12 +41,12 @@ public class TableAdminClientIT {
4141
static TableAdminClient tableAdmin;
4242

4343
@BeforeClass
44-
public static void setup() throws IOException {
44+
public static void setUp() throws IOException {
4545
tableAdmin = TableAdminClient.create(InstanceName.of("sduskis-hello-shakespear", "beam-test"));
4646
}
4747

4848
@AfterClass
49-
public static void cleanup() throws Exception {
49+
public static void cleanUp() throws Exception {
5050
tableAdmin.close();
5151
}
5252

@@ -90,20 +90,20 @@ public void modifyFamilies() {
9090
Duration.ofSeconds(1000);
9191
modifyFamiliesReq
9292
.create("mf1")
93-
.createWithGCRule("mf2", GCRULES.maxAge(Duration.ofSeconds(1000, 20000)))
94-
.updateWithGCRule(
93+
.create("mf2", GCRULES.maxAge(Duration.ofSeconds(1000, 20000)))
94+
.update(
9595
"mf1",
9696
GCRULES
9797
.union()
9898
.rule(GCRULES.maxAge(Duration.ofSeconds(100)))
9999
.rule(GCRULES.maxVersions(1)))
100-
.createWithGCRule(
100+
.create(
101101
"mf3",
102102
GCRULES
103103
.intersection()
104104
.rule(GCRULES.maxAge(Duration.ofSeconds(2000)))
105105
.rule(GCRULES.maxVersions(10)))
106-
.createWithGCRule(
106+
.create(
107107
"mf4", GCRULES.intersection().rule(GCRULES.maxAge(Duration.ofSeconds(360))))
108108
.create("mf5")
109109
.create("mf6")
@@ -224,7 +224,7 @@ public void dropRowRange() {
224224
try {
225225
tableAdmin.createTable(TableAdminRequests.createTable(tableId));
226226
tableAdmin.dropRowRange(tableId, "rowPrefix");
227-
tableAdmin.dropAllData(tableId);
227+
tableAdmin.dropAllRows(tableId);
228228
} finally {
229229
tableAdmin.deleteTable(tableId);
230230
}

0 commit comments

Comments
 (0)