fixed method names changes #3
Conversation
| buildCredentialProvider(builder, options.getCredentialOptions()); | ||
|
|
||
| buildBulkOptions(builder, options); | ||
| buildBulkMutations(builder, options); |
There was a problem hiding this comment.
method name like buildBulkMutationsOptions sounds better I think
There was a problem hiding this comment.
Options might confuse the use, I guess, would buildBulkMutationsSettings sound good?.. as in GCJ these are referred as settings
| "Disabling retries is not currently supported."); | ||
| } | ||
| if(options.getRetryOptions().allowRetriesWithoutTimestamp()) { | ||
| throw new UnsupportedOperationException("Please use unsafe Mutation method"); |
There was a problem hiding this comment.
Related to Review Comment about unsafe mutation
@ajaaym , I Will add more appropriate message, just want to confirm which factory method to point to.
| FixedHeaderProvider.create(GrpcUtil.USER_AGENT_KEY.name(), options.getUserAgent())); | ||
|
|
||
| // TODO: implementation for channelCount or channelPerCPU | ||
| ManagedChannelBuilder channelBuilder = ManagedChannelBuilder |
There was a problem hiding this comment.
Related to UserAgent comment
I have verified using debug mode, User Agent is being preserved in TransportChannelProvider. So that is the way to confirm, right?
There was a problem hiding this comment.
Having userAgenet value preserved in TransportChannel is good.
| .setInitialRpcTimeout(readPartialRowTimeout) | ||
| .setMaxRpcTimeout(readPartialRowTimeout) | ||
| .setTotalTimeout(ofMillis(options.getCallOptionsConfig().getLongRpcTimeoutMs())); | ||
|
|
There was a problem hiding this comment.
Related to readPatialRowsTimeout
After reading his comment, My approach here is to set RetrySettings separately for ReadRows & use buildIdempotentRetrySettings for sampleRowKeys & mutateRow (Assuming only these two are non streaming methods).
| buildCredentialProvider(builder, options.getCredentialOptions()); | ||
|
|
||
| buildBulkOptions(builder, options); | ||
| buildBulkMutations(builder, options); |
There was a problem hiding this comment.
Options might confuse the use, I guess, would buildBulkMutationsSettings sound good?.. as in GCJ these are referred as settings
| // TODO: an option to set RetryOptions#allowRetriesWithoutTimestamp | ||
|
|
||
| if (retryOptions.allowRetriesWithoutTimestamp()) { | ||
| LOG.warn("Retries without Timestamp doesn't support, please use Mutation#createUnsafe"); |
There was a problem hiding this comment.
@ajaaym I remember Solomon told about hbase's Mutation but I didn't any unsafe mutation in hbase's Mutation.. Could you please tell what method should we log.. or "just not support" is enough?
|
updated code for all feedback points |
* fixed method names changes * refactored code based on feedback * added logger & precondition * fixed names & error messages
only adding fromBigtableOptions static method adding separate methods for each settings Added check for retry & remvoed unneeded setter fixed method names changes (#3) * fixed method names changes * refactored code based on feedback * added logger & precondition * fixed names & error messages fixed retryCheck condition added test cases
only adding fromBigtableOptions static method adding separate methods for each settings Added check for retry & remvoed unneeded setter fixed method names changes (#3) * fixed method names changes * refactored code based on feedback * added logger & precondition * fixed names & error messages fixed retryCheck condition added test cases static method for BigtableDataSettings creation only adding fromBigtableOptions static method added test cases Adding BigtableDataClient & BigtableAdminClient renamed Factory methods Reverted BulkMutationSettings changes refactored Credentail Provider Server Test case for user-agent server test case for user-agent javadoc feedback update Adding language fix reviewed by Romlogic Renamed to Veneer Settings. adding ssl n plaintext test cases Adding ssl certificates
To verify all DataSettings related changes, before adding it into master repo.
This is for review purpose only