You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@trafficcontrol.apache.org by GitBox <gi...@apache.org> on 2021/09/15 19:12:08 UTC
[GitHub] [trafficcontrol] rawlinp commented on a change in pull request #6210: Update Traffic Router Java version to 11
rawlinp commented on a change in pull request #6210:
URL: https://github.com/apache/trafficcontrol/pull/6210#discussion_r709366413
##########
File path: traffic_router/core/src/main/java/org/apache/traffic_control/traffic_router/core/dns/DNSAccessEventBuilder.java
##########
@@ -145,12 +145,12 @@ public static String create(final DNSAccessRecord dnsAccessRecord, final Excepti
.append(" rdtl=-")
.append(" rerr=\"")
.append(rerr)
- .append("\"")
+ .append('\"')
.append(" ttl=\"-\"")
.append(" ans=\"-\"")
.append(" svc=\"")
.append(dsID)
- .append("\"").toString();
+ .append('\"').toString();
Review comment:
Does this need escaped since it's now a char instead of a string?
##########
File path: traffic_router/core/src/main/java/org/apache/traffic_control/traffic_router/core/ds/DeliveryService.java
##########
@@ -118,6 +118,7 @@
ALWAYS
}
+ @SuppressWarnings("PMD.NPathComplexity")
Review comment:
Just for my own edification, does suppressing warnings at the class level just suppress it for all methods in the class?
##########
File path: traffic_router/core/src/main/java/org/apache/traffic_control/traffic_router/core/dns/protocol/TCP.java
##########
@@ -46,9 +46,9 @@ public ServerSocket getServerSocket() {
@Override
public void run() {
while (!isShutdownRequested()) {
- try {
- final Socket socket = getServerSocket().accept();
- final TCPSocketHandler handler = new TCPSocketHandler(socket);
+ final TCPSocketHandler handler;
+ try (Socket socket = getServerSocket().accept()) {
Review comment:
Won't this prematurely close the socket before it's even handled?
##########
File path: .github/actions/build-rpms/build-rpms.sh
##########
@@ -28,9 +28,12 @@ if [[ "$GITHUB_REF" == refs/pull/*/merge ]]; then
pr_number="$(<<<"$GITHUB_REF" grep -o '[0-9]\+')"
files_changed="$(curl "${GITHUB_API_URL}/repos/${GITHUB_REPOSITORY}/pulls/${pr_number}/files" | jq -r .[].filename)"
else
- files_changed="$(git diff-tree --no-commit-id --name-only -r "$GITHUB_SHA")"
+ files_changed="$(git diff --name-only HEAD~4.. --)"
Review comment:
Just for my own edification -- what does this change do? Is this only diffing the last 4 commits?
##########
File path: traffic_router/core/src/main/java/org/apache/traffic_control/traffic_router/core/dns/DNSAccessEventBuilder.java
##########
@@ -122,12 +122,12 @@ public static String create(final DNSAccessRecord dnsAccessRecord, final WirePar
.append(" rdtl=-")
.append(" rerr=\"")
.append(rerr)
- .append("\"")
+ .append('\"')
.append(" ttl=\"-\"")
.append(" ans=\"-\"")
.append(" svc=\"")
.append(dsID)
- .append("\"")
+ .append('\"')
Review comment:
Does this need escaped since it's now a char instead of a string?
##########
File path: traffic_router/core/src/main/java/org/apache/traffic_control/traffic_router/core/dns/DNSAccessEventBuilder.java
##########
@@ -122,12 +122,12 @@ public static String create(final DNSAccessRecord dnsAccessRecord, final WirePar
.append(" rdtl=-")
.append(" rerr=\"")
.append(rerr)
- .append("\"")
+ .append('\"')
Review comment:
Does this need escaped since it's now a char instead of a string?
##########
File path: traffic_router/core/src/main/java/org/apache/traffic_control/traffic_router/core/ds/LetsEncryptDnsChallengeWatcher.java
##########
@@ -128,6 +128,7 @@ public String getWatcherConfigPrefix() {
return "dnschallengemapping";
}
+ @SuppressWarnings("PMD.CloseResource")
Review comment:
Why suppress here but not others?
##########
File path: traffic_router/core/src/main/java/org/apache/traffic_control/traffic_router/core/dns/DNSAccessEventBuilder.java
##########
@@ -145,12 +145,12 @@ public static String create(final DNSAccessRecord dnsAccessRecord, final Excepti
.append(" rdtl=-")
.append(" rerr=\"")
.append(rerr)
- .append("\"")
+ .append('\"')
Review comment:
Does this need escaped since it's now a char instead of a string?
--
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@trafficcontrol.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org