[GitHub] [iceberg] findepi commented on pull request #6474: Make it explicit that metrics reporter is required

[GitBox <gi...@apache.org>]

findepi commented on PR #6474:
URL: https://github.com/apache/iceberg/pull/6474#issuecomment-1396954141

   > looks good to me
   
   thanks for your review @jackye1995 !
   
   >  would be good if we reach some consensus about the way for null check
   
   i prefer `checkNotNull`, but it's up for the project to decide
   i just hope this future decision isn't required for the merge here
   
   BTW, I am  the library's  user more frequently than its contributor. As a user, I would benefit from explicit indication which API methods accept nulls, and even more -- which can return nulls.
   We keep asking "can this be null" question in PR reviews (eg here https://github.com/trinodb/trino/pull/14869#discussion_r1072388756)
   but more often than not, we _forget_ to ask that question, potentially setting us to NPE failures.
   
   This too, I would prefer to be a follow up, not a prerequisite for a merge here.


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

To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org