ndimiduk commented on a change in pull request #2862:
URL: https://github.com/apache/hbase/pull/2862#discussion_r554189525



##########
File path: dev-support/checkcompatibility.py
##########
@@ -241,6 +241,20 @@ def compare_results(tool_results, known_issues, 
compare_warnings):
 
     return bool(unexpected_issues)
 
+def compare_tool_results_count(tool_results, check, issue_type, known_count):
+    """ Check problem counts are no more than the known count.
+    (This function exists just so can add in logging; previous was inlined
+    one-liner but this made it hard debugging)
+    """
+    # logging.info("known_count=%s, check key=%s, tool_results=%s, 
issue_type=%s",
+    #        str(known_count), str(check), str(tool_results), str(issue_type))
+    try:
+       return tool_results[check][issue_type] > known_count
+    except KeyError as ke:
+      logging.info("KeyError %s", str(ke))

Review comment:
       why log + swallow the error case here? Isn't it better to `raise` the 
error instead of just logging it? Or maybe calling code ignores thrown 
exceptions?

##########
File path: dev-support/create-release/release-util.sh
##########
@@ -469,8 +469,18 @@ function generate_api_report {
   local timing_token
   timing_token="$(start_step)"
   # Generate api report.
+  # Filter out some jar types. Filters are tricky. Python regex on
+  # file basename. Exclude the saved-aside original jars... they are
+  # not included in resulting artifact. Also, do not include the
+  # hbase-shaded-testing-util.*  jars. This jar is unzip'able on mac
+  # os x as is because has it a META_INF/LICENSE file and then a
+  # META_INF/license directory for the included jar's licenses;
+  # it fails to unjar on mac os x which this tool does making its checks
+  # (Its exclusion should be fine; it is just an aggregate of other jars).
   "${project}"/dev-support/checkcompatibility.py --annotation \
     org.apache.yetus.audience.InterfaceAudience.Public  \
+    -e "original-hbase.*.jar" \
+    -e "hbase-shaded-testing-util.*.jar" \

Review comment:
       excluding this jar is a problem -- the shaded jars are intended 
explicitly as our public interface to down-streamers, aren't they?
   
   What if we add an exclude of the `[lL][iI][cC][eE][nN][sS][eE]` pattern 
instead? oh, I see the `jar` command doesn't support exclude patterns, only an 
include pattern. And where does this happen, in the perl script?

##########
File path: dev-support/create-release/release-util.sh
##########
@@ -469,8 +469,18 @@ function generate_api_report {
   local timing_token
   timing_token="$(start_step)"
   # Generate api report.
+  # Filter out some jar types. Filters are tricky. Python regex on
+  # file basename. Exclude the saved-aside original jars... they are
+  # not included in resulting artifact. Also, do not include the
+  # hbase-shaded-testing-util.*  jars. This jar is unzip'able on mac
+  # os x as is because has it a META_INF/LICENSE file and then a
+  # META_INF/license directory for the included jar's licenses;
+  # it fails to unjar on mac os x which this tool does making its checks
+  # (Its exclusion should be fine; it is just an aggregate of other jars).
   "${project}"/dev-support/checkcompatibility.py --annotation \
     org.apache.yetus.audience.InterfaceAudience.Public  \
+    -e "original-hbase.*.jar" \
+    -e "hbase-shaded-testing-util.*.jar" \

Review comment:
       > Include/exclude are in the python script. Does jar files only. The jar 
files are subsequently passed to the wrapped perl script... which then does 
unzip... and fails with hbase-shaded-testing-util.
   
   I meant exclude of unpacked archive content, not entire archives. And yes, 
that's what I feared, that the unpack happens in the perl script, "outside of 
our control."
   
   I'm thinking the reason to retain the shaded jar is to notice if we break 
shading entirely. i.e., a change that adds a new jar to the shaded package 
would show in the report as loads of new classes. Perhaps we have other 
mechanisms in place already for catching these kinds of errors?
   
   I like HBASE-25486.




----------------------------------------------------------------
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


Reply via email to