Jackie-Jiang commented on a change in pull request #7308:
URL: https://github.com/apache/pinot/pull/7308#discussion_r689945159
##########
File path:
pinot-spi/src/main/java/org/apache/pinot/spi/config/table/assignment/InstanceReplicaGroupPartitionConfig.java
##########
@@ -41,7 +41,8 @@
@JsonPropertyDescription("Number of partitions for replica-group based
selection, do not partition the replica-group (1 partition) if not specified")
private final int _numPartitions;
- @JsonPropertyDescription("Number of instances per partition (within a
replica-group) for replica-group based selection, select all instances if not
specified")
+ @JsonPropertyDescription(
+ "Number of instances per partition (within a replica-group) for
replica-group based selection, select all instances if not specified")
Review comment:
This line is still too long?
##########
File path:
pinot-spi/src/main/java/org/apache/pinot/spi/config/table/assignment/InstanceAssignmentConfig.java
##########
@@ -31,10 +31,12 @@
@JsonPropertyDescription("Configuration for the instance tag and pool of the
instance assignment (mandatory)")
private final InstanceTagPoolConfig _tagPoolConfig;
- @JsonPropertyDescription("Configuration for the instance constraints of the
instance assignment, which filters out unqualified instances and sorts
instances for picking priority")
+ @JsonPropertyDescription("Configuration for the instance constraints of the
instance assignment,"
+ + " which filters out unqualified instances and sorts instances for
picking priority")
private final InstanceConstraintConfig _constraintConfig;
- @JsonPropertyDescription("Configuration for the instance replica-group and
partition of the instance assignment (mandatory)")
+ @JsonPropertyDescription(
+ "Configuration for the instance replica-group and partition of" + " the
instance assignment (mandatory)")
Review comment:
This looks slightly weird. One way to fix is to add empty comment `//`
to the first line to force it to change the line
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/data/Schema.java
##########
@@ -479,6 +479,7 @@ public void validate() {
default:
throw new IllegalStateException("Unsupported data type: " +
dataType + " in COMPLEX field: " + fieldName);
}
+ break;
Review comment:
+1
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/data/Schema.java
##########
@@ -839,6 +840,8 @@ private static String
constructTransformFunctionString(String incomingName, int
outerFunction = String.format("toEpochDays(%s)", innerFunction);
}
break;
+ default:
+ throw new RuntimeException("Unsupported outgoingTimeUnit - " +
outgoingTimeUnit);
Review comment:
Same
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/data/Schema.java
##########
@@ -804,11 +804,12 @@ private static String
constructTransformFunctionString(String incomingName, int
innerFunction = String.format("fromEpochDays(%s)", incomingName);
}
break;
+ default:
+ throw new RuntimeException("Unsupported incomingTimeUnit - " +
incomingTimeUnit);
Review comment:
We usually throw `IllegalStateException` here
##########
File path:
pinot-spi/src/main/java/org/apache/pinot/spi/env/PinotConfiguration.java
##########
@@ -92,17 +92,17 @@ public PinotConfiguration() {
/**
* Builds a new instance out of an existing Apache Commons {@link
Configuration} instance. Will apply property key sanitization to enable relaxed
binding.
* Properties from the base configuration will be copied and won't be linked.
- *
+ *
* @param baseConfiguration an existing {@link Configuration} for which all
properties will be duplicated and sanitized for relaxed binding.
*/
public PinotConfiguration(Configuration baseConfiguration) {
- this.configuration =
+ this._configuration =
Review comment:
Remove `this.`, same for other places
##########
File path: pinot-spi/src/main/java/org/apache/pinot/spi/config/ConfigUtils.java
##########
@@ -31,6 +31,9 @@
public class ConfigUtils {
private static final Map<String, String> ENVIRONMENT_VARIABLES =
System.getenv();
+ private ConfigUtils() {
Review comment:
(nit) We usually put this right next to the class definition for
slightly better readability, see other utils class for example. Same for other
util classes
--
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]
