kbendick commented on code in PR #4596:
URL: https://github.com/apache/iceberg/pull/4596#discussion_r904514624
##########
core/src/main/java/org/apache/iceberg/SystemProperties.java:
##########
@@ -39,11 +41,26 @@ private SystemProperties() {
*/
public static final String SCAN_THREAD_POOL_ENABLED =
"iceberg.scan.plan-in-worker-pool";
+ /**
+ * Sets the size of the queue, which is used to avoid consuming too much
memory.
+ */
+ public static final String SCAN_SHARED_QUEUE_SIZE =
"iceberg.scan.shared-queue-size";
+ public static final int SCAN_SHARED_QUEUE_SIZE_DEFAULT = 1000;
+
static boolean getBoolean(String systemProperty, boolean defaultValue) {
String value = System.getProperty(systemProperty);
if (value != null) {
return Boolean.parseBoolean(value);
}
return defaultValue;
}
+
+ public static int getInt(String systemProperty, int defaultValue) {
+ Preconditions.checkNotNull(systemProperty, "System property name should
not be null");
Review Comment:
General but / FYI - we almost always tend to prefer plain English language
with a structure similar to “general type of problem / problem phrase [possible
solution only if not clear from stack trace]: offending value”.
we put the bad value at the end after a colon to give us a more continuous
search string for logs (instead of mixing them into the sentence).
So for this situation, the suggested sentence would be format(“Invalid value
for system property %s: null”, systemProperty);
However for this we can be more specific as null here means missing / not
set almost always (and anybody who set it to null will see that). So I’d
suggest format(“Invalid value for system property %s: null”, systemProperty);
The final `: null` is debatable but I’d personally put it just to cover any
cases where it might be explicitly null and just to match many others
preconditions.
Yours is pretty good, but this tends to be our standard. I think it’s most
helpful thinking of it in the context of the NPE or IllegalArumentException and
full stack trace. What’s invariably helpful is having a common-ish way of
writing these with a long enough, specific enough stack trace to be searchable,
with less and less specific search query as the phrase is shortened.
Hope that helps for the long term! It really is just a nit but if you’re
working on Iceberg offen enough it might help you as a contributor and a user.
```suggestion
Preconditions.checkNotNull(systemProperty, String.format("Invalid value
for system property %s: null", systemProperty);
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
