[GitHub] [hbase] bitoffdev commented on a change in pull request #1959: HBASE-20819 Use TableDescriptor to replace HTableDescriptor in hbase-shell module
bitoffdev commented on a change in pull request #1959: URL: https://github.com/apache/hbase/pull/1959#discussion_r446296466 ## File path: hbase-shell/src/main/ruby/hbase_constants.rb ## @@ -109,8 +109,8 @@ def self.promote_constants(constants) end end - promote_constants(org.apache.hadoop.hbase.HColumnDescriptor.constants) - promote_constants(org.apache.hadoop.hbase.HTableDescriptor.constants) + promote_constants(org.apache.hadoop.hbase.client.ColumnFamilyDescriptorBuilder.constants) Review comment: I just pushed a commit that adds back three constants, but leaves the rest out. I left a [comment on Jira](https://issues.apache.org/jira/browse/HBASE-20819?focusedCommentId=17146470#comment-17146470) with my rationale for each of the constants. **Let me know if you disagree with any of my conclusions, and I can easily add more constants back** . Specifically for the [MOB constants](https://github.com/apache/hbase/blob/025ddce868eb06b4072b5152c5ffae5a01e7ae30/hbase-client/src/main/java/org/apache/hadoop/hbase/client/ColumnFamilyDescriptorBuilder.java#L163), we added setters for all the MOB key-value pairs to our builder. I think including these _private_ constants will encourage users to go against our defined interface. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [hbase] bitoffdev commented on a change in pull request #1959: HBASE-20819 Use TableDescriptor to replace HTableDescriptor in hbase-shell module
bitoffdev commented on a change in pull request #1959:
URL: https://github.com/apache/hbase/pull/1959#discussion_r445838230
##
File path: hbase-shell/src/main/ruby/hbase/admin.rb
##
@@ -971,101 +976,103 @@ def enabled?(table_name)
end
#--
-# Return a new HColumnDescriptor made of passed args
-def hcd(arg, htd)
+# Return a new ColumnFamilyDescriptor made of passed args
+def hcd(arg, tdb)
# String arg, single parameter constructor
- return org.apache.hadoop.hbase.HColumnDescriptor.new(arg) if
arg.is_a?(String)
+
+ return ColumnFamilyDescriptorBuilder.of(arg) if arg.is_a?(String)
raise(ArgumentError, "Column family #{arg} must have a name") unless
name = arg.delete(NAME)
- family = htd.getFamily(name.to_java_bytes)
+ cfd = tdb.build.getColumnFamily(name.to_java_bytes)
Review comment:
I think that if we do choose to add any sort of introspection (like
hasColumnFamily, getColumnFamily) to the TableDescriptorBuilder itself, that
would belong in a separate ticket/issue.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [hbase] bitoffdev commented on a change in pull request #1959: HBASE-20819 Use TableDescriptor to replace HTableDescriptor in hbase-shell module
bitoffdev commented on a change in pull request #1959:
URL: https://github.com/apache/hbase/pull/1959#discussion_r445790978
##
File path: hbase-shell/src/main/ruby/hbase/admin.rb
##
@@ -971,101 +976,103 @@ def enabled?(table_name)
end
#--
-# Return a new HColumnDescriptor made of passed args
-def hcd(arg, htd)
+# Return a new ColumnFamilyDescriptor made of passed args
+def hcd(arg, tdb)
# String arg, single parameter constructor
- return org.apache.hadoop.hbase.HColumnDescriptor.new(arg) if
arg.is_a?(String)
+
+ return ColumnFamilyDescriptorBuilder.of(arg) if arg.is_a?(String)
raise(ArgumentError, "Column family #{arg} must have a name") unless
name = arg.delete(NAME)
- family = htd.getFamily(name.to_java_bytes)
+ cfd = tdb.build.getColumnFamily(name.to_java_bytes)
Review comment:
I think this is the best way to do it at the moment. Currently, it seems
that TableDescriptorBuilder is intended to be used for writing only, in which
case we would not want the builder itself to have methods like getColumnFamily
or hasColumnFamily. This is consistent with the lack of getValue and hasValue
methods on the builder.
Other than adding methods to builder, the only other way to shortcut the
handful of calls this patch makes to `tdb.build` would be to cache the
TableDescriptor at the start of each method that uses a T.D.. I have to
recommend against this approach since it would technically change the behavior.
Ex: If you were to execute something in the shell like `alter 't1', {NAME =>
'fam1', METHOD => 'delete'}, {NAME => 'fam1', VERSIONS => 5}` where a C.F. is
changed multiple times. In this example, a cached T.D. would not reflect the
deletion of the C.F..
**With these thoughts, I am inclined to leave the patch as-is.**
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [hbase] bitoffdev commented on a change in pull request #1959: HBASE-20819 Use TableDescriptor to replace HTableDescriptor in hbase-shell module
bitoffdev commented on a change in pull request #1959:
URL: https://github.com/apache/hbase/pull/1959#discussion_r445790978
##
File path: hbase-shell/src/main/ruby/hbase/admin.rb
##
@@ -971,101 +976,103 @@ def enabled?(table_name)
end
#--
-# Return a new HColumnDescriptor made of passed args
-def hcd(arg, htd)
+# Return a new ColumnFamilyDescriptor made of passed args
+def hcd(arg, tdb)
# String arg, single parameter constructor
- return org.apache.hadoop.hbase.HColumnDescriptor.new(arg) if
arg.is_a?(String)
+
+ return ColumnFamilyDescriptorBuilder.of(arg) if arg.is_a?(String)
raise(ArgumentError, "Column family #{arg} must have a name") unless
name = arg.delete(NAME)
- family = htd.getFamily(name.to_java_bytes)
+ cfd = tdb.build.getColumnFamily(name.to_java_bytes)
Review comment:
I think this is the best way to do it at the moment. Currently, it seems
that TableDescriptorBuilder is intended to be used for writing only, in which
case we would not want the builder itself to have methods like getColumnFamily
or hasColumnFamily. This is consistent with the lack of getValue and setValue
methods on the builder.
Other than adding methods to builder, the only other way to shortcut the
handful of calls this patch makes to `tdb.build` would be to cache the
TableDescriptor at the start of each method that uses a T.D.. I have to
recommend against this approach since it would technically change the behavior.
Ex: If you were to execute something in the shell like `alter 't1', {NAME =>
'fam1', METHOD => 'delete'}, {NAME => 'fam1', VERSIONS => 5}` where a C.F. is
changed multiple times. In this example, a cached T.D. would not reflect the
deletion of the C.F..
**With these thoughts, I am inclined to leave the patch as-is.**
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [hbase] bitoffdev commented on a change in pull request #1959: HBASE-20819 Use TableDescriptor to replace HTableDescriptor in hbase-shell module
bitoffdev commented on a change in pull request #1959: URL: https://github.com/apache/hbase/pull/1959#discussion_r445722179 ## File path: hbase-shell/src/main/ruby/hbase/admin.rb ## @@ -1359,26 +1366,26 @@ def list_locks @admin.getLocks end -# Parse arguments and update HTableDescriptor accordingly -def update_htd_from_arg(htd, arg) Review comment: Thanks for the heads up! I just updated the "Release Note" field in Jira to state that this method was removed. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [hbase] bitoffdev commented on a change in pull request #1959: HBASE-20819 Use TableDescriptor to replace HTableDescriptor in hbase-shell module
bitoffdev commented on a change in pull request #1959: URL: https://github.com/apache/hbase/pull/1959#discussion_r445746679 ## File path: hbase-shell/src/main/ruby/hbase_constants.rb ## @@ -109,8 +109,8 @@ def self.promote_constants(constants) end end - promote_constants(org.apache.hadoop.hbase.HColumnDescriptor.constants) - promote_constants(org.apache.hadoop.hbase.HTableDescriptor.constants) + promote_constants(org.apache.hadoop.hbase.client.ColumnFamilyDescriptorBuilder.constants) Review comment: This patch removes 11 constants. I added these to the release notes and left a long-form explanation in Jira: https://issues.apache.org/jira/browse/HBASE-20819?focusedCommentId=17145696 This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [hbase] bitoffdev commented on a change in pull request #1959: HBASE-20819 Use TableDescriptor to replace HTableDescriptor in hbase-shell module
bitoffdev commented on a change in pull request #1959: URL: https://github.com/apache/hbase/pull/1959#discussion_r445722179 ## File path: hbase-shell/src/main/ruby/hbase/admin.rb ## @@ -1359,26 +1366,26 @@ def list_locks @admin.getLocks end -# Parse arguments and update HTableDescriptor accordingly -def update_htd_from_arg(htd, arg) Review comment: Thanks for the heads up! I just update the "Release Note" field in Jira to state that this method was removed. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [hbase] bitoffdev commented on a change in pull request #1959: HBASE-20819 Use TableDescriptor to replace HTableDescriptor in hbase-shell module
bitoffdev commented on a change in pull request #1959: URL: https://github.com/apache/hbase/pull/1959#discussion_r445721365 ## File path: hbase-shell/src/main/ruby/hbase/admin.rb ## @@ -971,101 +976,103 @@ def enabled?(table_name) end #-- -# Return a new HColumnDescriptor made of passed args -def hcd(arg, htd) +# Return a new ColumnFamilyDescriptor made of passed args +def hcd(arg, tdb) Review comment: I agree. I went ahead and renamed `hcd` to `cfd` and added the removal of `hcd` to the custom "Release Note" field in Jira. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [hbase] bitoffdev commented on a change in pull request #1959: HBASE-20819 Use TableDescriptor to replace HTableDescriptor in hbase-shell module
bitoffdev commented on a change in pull request #1959:
URL: https://github.com/apache/hbase/pull/1959#discussion_r445013274
##
File path: hbase-shell/src/main/ruby/hbase/admin.rb
##
@@ -701,40 +703,40 @@ def alter(table_name_str, wait = true, *args)
# Delete column family
if method == 'delete'
raise(ArgumentError, 'NAME parameter missing for delete method')
unless name
-htd.removeFamily(name.to_java_bytes)
+tdb.removeColumnFamily(name.to_java_bytes)
hasTableUpdate = true
# Unset table attributes
elsif method == 'table_att_unset'
raise(ArgumentError, 'NAME parameter missing for table_att_unset
method') unless name
if name.is_a?(Array)
name.each do |key|
-if htd.getValue(key).nil?
+if tdb.build.getValue(key).nil?
raise ArgumentError, "Could not find attribute: #{key}"
end
-htd.remove(key)
+tdb.setValue(key, nil)
end
else
- if htd.getValue(name).nil?
+ if tdb.build.getValue(name).nil?
raise ArgumentError, "Could not find attribute: #{name}"
end
- htd.remove(name)
+ tdb.setValue(name, nil)
Review comment:
Done
##
File path: hbase-shell/src/main/ruby/hbase/admin.rb
##
@@ -701,40 +703,40 @@ def alter(table_name_str, wait = true, *args)
# Delete column family
if method == 'delete'
raise(ArgumentError, 'NAME parameter missing for delete method')
unless name
-htd.removeFamily(name.to_java_bytes)
+tdb.removeColumnFamily(name.to_java_bytes)
hasTableUpdate = true
# Unset table attributes
elsif method == 'table_att_unset'
raise(ArgumentError, 'NAME parameter missing for table_att_unset
method') unless name
if name.is_a?(Array)
name.each do |key|
-if htd.getValue(key).nil?
+if tdb.build.getValue(key).nil?
raise ArgumentError, "Could not find attribute: #{key}"
end
-htd.remove(key)
+tdb.setValue(key, nil)
end
else
- if htd.getValue(name).nil?
+ if tdb.build.getValue(name).nil?
raise ArgumentError, "Could not find attribute: #{name}"
end
- htd.remove(name)
+ tdb.setValue(name, nil)
end
hasTableUpdate = true
# Unset table configuration
elsif method == 'table_conf_unset'
raise(ArgumentError, 'NAME parameter missing for table_conf_unset
method') unless name
if name.is_a?(Array)
name.each do |key|
-if htd.getConfigurationValue(key).nil?
+if tdb.build.getValue(key).nil?
raise ArgumentError, "Could not find configuration: #{key}"
end
-htd.removeConfiguration(key)
+tdb.setValue(key, nil)
Review comment:
Done
##
File path: hbase-shell/src/main/ruby/hbase/admin.rb
##
@@ -701,40 +703,40 @@ def alter(table_name_str, wait = true, *args)
# Delete column family
if method == 'delete'
raise(ArgumentError, 'NAME parameter missing for delete method')
unless name
-htd.removeFamily(name.to_java_bytes)
+tdb.removeColumnFamily(name.to_java_bytes)
hasTableUpdate = true
# Unset table attributes
elsif method == 'table_att_unset'
raise(ArgumentError, 'NAME parameter missing for table_att_unset
method') unless name
if name.is_a?(Array)
name.each do |key|
-if htd.getValue(key).nil?
+if tdb.build.getValue(key).nil?
raise ArgumentError, "Could not find attribute: #{key}"
end
-htd.remove(key)
+tdb.setValue(key, nil)
end
else
- if htd.getValue(name).nil?
+ if tdb.build.getValue(name).nil?
raise ArgumentError, "Could not find attribute: #{name}"
end
- htd.remove(name)
+ tdb.setValue(name, nil)
end
hasTableUpdate = true
# Unset table configuration
elsif method == 'table_conf_unset'
raise(ArgumentError, 'NAME parameter missing for table_conf_unset
method') unless name
if name.is_a?(Array)
name.each do |key|
-if htd.getConfigurationValue(key).nil?
+if tdb.build.getValue(key).nil?
raise ArgumentError, "Could not find configuration:
[GitHub] [hbase] bitoffdev commented on a change in pull request #1959: HBASE-20819 Use TableDescriptor to replace HTableDescriptor in hbase-shell module
bitoffdev commented on a change in pull request #1959: URL: https://github.com/apache/hbase/pull/1959#discussion_r445012104 ## File path: hbase-shell/src/main/ruby/hbase/admin.rb ## @@ -25,6 +25,8 @@ java_import org.apache.hadoop.hbase.util.Bytes java_import org.apache.hadoop.hbase.ServerName java_import org.apache.hadoop.hbase.TableName +java_import org.apache.hadoop.hbase.client.ColumnFamilyDescriptorBuilder Review comment: As of my most recent commit, the symbols HTableDescriptor and HColumnDescriptor are not present in the hbase-shell module, which I verified via `grep -r 'HColumnDescriptor\|HTableDescriptor' hbase-shell` This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [hbase] bitoffdev commented on a change in pull request #1959: HBASE-20819 Use TableDescriptor to replace HTableDescriptor in hbase-shell module
bitoffdev commented on a change in pull request #1959:
URL: https://github.com/apache/hbase/pull/1959#discussion_r444922986
##
File path: hbase-shell/src/main/ruby/hbase/admin.rb
##
@@ -701,40 +703,40 @@ def alter(table_name_str, wait = true, *args)
# Delete column family
if method == 'delete'
raise(ArgumentError, 'NAME parameter missing for delete method')
unless name
-htd.removeFamily(name.to_java_bytes)
+tdb.removeColumnFamily(name.to_java_bytes)
hasTableUpdate = true
# Unset table attributes
elsif method == 'table_att_unset'
raise(ArgumentError, 'NAME parameter missing for table_att_unset
method') unless name
if name.is_a?(Array)
name.each do |key|
-if htd.getValue(key).nil?
+if tdb.build.getValue(key).nil?
raise ArgumentError, "Could not find attribute: #{key}"
end
-htd.remove(key)
+tdb.setValue(key, nil)
Review comment:
Unfortunately, the current overloads for
`TableDescriptorBuilder.removeValue` take either `Bytes`, or `byte[]`, whereas
`setValue` has an overload for a String argument. **Unless there is an
objection, I will go ahead and add the `removeValue(final String key)`
overload**, which would be consistent with the `setValue` overloads, and may be
helpful for other developers in the future as well.
The other alternative (converting the key to bytes) seems like it would be
more effort than just using `setValue` as is.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
