You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by GitBox <gi...@apache.org> on 2022/07/15 09:05:22 UTC
[GitHub] [flink] reswqa commented on a diff in pull request #20280: [FLINK-28558][history] Improve log-url configuration usability.
reswqa commented on code in PR #20280:
URL: https://github.com/apache/flink/pull/20280#discussion_r921932146
##########
flink-runtime-web/src/main/java/org/apache/flink/runtime/webmonitor/history/HistoryServer.java:
##########
@@ -291,23 +290,24 @@ void start() throws IOException, InterruptedException {
Router router = new Router();
- final String jobManagerPattern =
- config.get(HistoryServerOptions.HISTORY_SERVER_JOBMANAGER_LOG_URL_PATTERN);
- if (!StringUtils.isNullOrWhitespaceOnly(jobManagerPattern)) {
- router.addGet(
- JobManagerLogUrlHeaders.getInstance().getTargetRestEndpointURL(),
- new GeneratedLogUrlHandler(
- CompletableFuture.completedFuture(jobManagerPattern)));
- }
-
- final String taskManagerPattern =
- config.get(HistoryServerOptions.HISTORY_SERVER_TASKMANAGER_LOG_URL_PATTERN);
- if (!StringUtils.isNullOrWhitespaceOnly(taskManagerPattern)) {
- router.addGet(
- TaskManagerLogUrlHeaders.getInstance().getTargetRestEndpointURL(),
- new GeneratedLogUrlHandler(
- CompletableFuture.completedFuture(taskManagerPattern)));
- }
+ LogUrlUtil.getValidLogUrlPattern(
+ config, HistoryServerOptions.HISTORY_SERVER_JOBMANAGER_LOG_URL_PATTERN)
+ .ifPresent(
+ pattern ->
+ router.addGet(
+ JobManagerLogUrlHeaders.getInstance()
+ .getTargetRestEndpointURL(),
+ new GeneratedLogUrlHandler(
+ CompletableFuture.completedFuture(pattern))));
+ LogUrlUtil.getValidLogUrlPattern(
+ config, HistoryServerOptions.HISTORY_SERVER_TASKMANAGER_LOG_URL_PATTERN)
+ .ifPresent(
+ pattern ->
+ router.addGet(
+ JobManagerLogUrlHeaders.getInstance()
Review Comment:
```suggestion
TaskManagerLogUrlHeaders.getInstance()
```
##########
flink-runtime-web/src/test/java/org/apache/flink/runtime/webmonitor/utils/LogUrlUtilTest.java:
##########
@@ -0,0 +1,76 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.flink.runtime.webmonitor.utils;
+
+import org.apache.flink.configuration.Configuration;
+
+import org.junit.Test;
Review Comment:
```suggestion
import org.junit.jupiter.api.Test;
```
Junit5 is best used for newly introduced tests.
##########
flink-runtime-web/src/test/java/org/apache/flink/runtime/webmonitor/utils/LogUrlUtilTest.java:
##########
@@ -0,0 +1,76 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.flink.runtime.webmonitor.utils;
+
+import org.apache.flink.configuration.Configuration;
+
+import org.junit.Test;
+
+import java.util.Optional;
+
+import static org.apache.flink.configuration.HistoryServerOptions.HISTORY_SERVER_JOBMANAGER_LOG_URL_PATTERN;
+import static org.apache.flink.configuration.HistoryServerOptions.HISTORY_SERVER_TASKMANAGER_LOG_URL_PATTERN;
+import static org.assertj.core.api.Assertions.assertThat;
+
+/** Tests for {@link LogUrlUtil}. */
+public class LogUrlUtilTest {
+
+ private static final String PATTERN_WITHOUT_SCHEME =
+ "my.testing-url?job=<jobid>&taskmanager=<tmid>";
+
+ @Test
+ public void testGetValidLogUrlPatternHttp() {
+ String pattern = "http://" + PATTERN_WITHOUT_SCHEME;
+ testGetValidLogUrlPattern(pattern, Optional.of(pattern));
+ }
+
+ @Test
+ public void testGetValidLogUrlPatternHttps() {
+ String pattern = "https://" + PATTERN_WITHOUT_SCHEME;
+ testGetValidLogUrlPattern(pattern, Optional.of(pattern));
+ }
+
+ @Test
+ public void testGetValidLogUrlPatternNoScheme() {
+ testGetValidLogUrlPattern(
+ PATTERN_WITHOUT_SCHEME, Optional.of("http://" + PATTERN_WITHOUT_SCHEME));
+ }
+
+ @Test
+ public void testGetValidLogUrlPatternUnsupportedScheme() {
+ testGetValidLogUrlPattern("file://" + PATTERN_WITHOUT_SCHEME, Optional.empty());
+ }
+
Review Comment:
I suggest adding another test with empty pattern or do not configure the corresponding options.
##########
flink-runtime-web/src/main/java/org/apache/flink/runtime/webmonitor/utils/LogUrlUtil.java:
##########
@@ -0,0 +1,65 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.flink.runtime.webmonitor.utils;
+
+import org.apache.flink.configuration.ConfigOption;
+import org.apache.flink.configuration.Configuration;
+import org.apache.flink.util.StringUtils;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.util.Optional;
+
+/** Util for log url pattern. */
+public class LogUrlUtil {
+
+ private static final String SCHEME_SEPARATOR = "://";
+ private static final String HTTP = "http";
+ private static final String HTTPS = "https";
+
+ private static final Logger LOG = LoggerFactory.getLogger(LogUrlUtil.class);
+
+ /** Validate and normalize log url pattern. */
+ public static Optional<String> getValidLogUrlPattern(
+ final Configuration config, final ConfigOption<String> option) {
+ String pattern = config.getString(option);
+ if (StringUtils.isNullOrWhitespaceOnly(pattern)) {
+ return Optional.empty();
+ }
+
+ pattern = pattern.trim();
+
+ String scheme = pattern.substring(0, Math.max(pattern.indexOf(SCHEME_SEPARATOR), 0));
+ if (scheme.isEmpty()) {
+ return Optional.of(HTTP + SCHEME_SEPARATOR + pattern);
+ } else if (HTTP.compareToIgnoreCase(scheme) == 0
Review Comment:
Maybe `equalsIgnoreCase` instead of `compareToIgnoreCase ` more intuitive.
##########
flink-runtime-web/src/main/java/org/apache/flink/runtime/webmonitor/utils/LogUrlUtil.java:
##########
@@ -0,0 +1,65 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.flink.runtime.webmonitor.utils;
+
+import org.apache.flink.configuration.ConfigOption;
+import org.apache.flink.configuration.Configuration;
+import org.apache.flink.util.StringUtils;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.util.Optional;
+
+/** Util for log url pattern. */
+public class LogUrlUtil {
+
+ private static final String SCHEME_SEPARATOR = "://";
+ private static final String HTTP = "http";
Review Comment:
Maybe HTTP_SCHEME and HTTPS_SCHEME or others name more appropriate than HTTP/HTTPS.
##########
flink-runtime-web/src/test/java/org/apache/flink/runtime/webmonitor/utils/LogUrlUtilTest.java:
##########
@@ -0,0 +1,76 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.flink.runtime.webmonitor.utils;
+
+import org.apache.flink.configuration.Configuration;
+
+import org.junit.Test;
+
+import java.util.Optional;
+
+import static org.apache.flink.configuration.HistoryServerOptions.HISTORY_SERVER_JOBMANAGER_LOG_URL_PATTERN;
+import static org.apache.flink.configuration.HistoryServerOptions.HISTORY_SERVER_TASKMANAGER_LOG_URL_PATTERN;
+import static org.assertj.core.api.Assertions.assertThat;
+
+/** Tests for {@link LogUrlUtil}. */
+public class LogUrlUtilTest {
+
+ private static final String PATTERN_WITHOUT_SCHEME =
+ "my.testing-url?job=<jobid>&taskmanager=<tmid>";
+
+ @Test
+ public void testGetValidLogUrlPatternHttp() {
Review Comment:
```suggestion
void testGetValidLogUrlPatternHttp() {
```
##########
flink-runtime-web/src/test/java/org/apache/flink/runtime/webmonitor/utils/LogUrlUtilTest.java:
##########
@@ -0,0 +1,76 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.flink.runtime.webmonitor.utils;
+
+import org.apache.flink.configuration.Configuration;
+
+import org.junit.Test;
+
+import java.util.Optional;
+
+import static org.apache.flink.configuration.HistoryServerOptions.HISTORY_SERVER_JOBMANAGER_LOG_URL_PATTERN;
+import static org.apache.flink.configuration.HistoryServerOptions.HISTORY_SERVER_TASKMANAGER_LOG_URL_PATTERN;
+import static org.assertj.core.api.Assertions.assertThat;
+
+/** Tests for {@link LogUrlUtil}. */
+public class LogUrlUtilTest {
+
+ private static final String PATTERN_WITHOUT_SCHEME =
+ "my.testing-url?job=<jobid>&taskmanager=<tmid>";
+
+ @Test
+ public void testGetValidLogUrlPatternHttp() {
+ String pattern = "http://" + PATTERN_WITHOUT_SCHEME;
+ testGetValidLogUrlPattern(pattern, Optional.of(pattern));
+ }
+
+ @Test
+ public void testGetValidLogUrlPatternHttps() {
+ String pattern = "https://" + PATTERN_WITHOUT_SCHEME;
+ testGetValidLogUrlPattern(pattern, Optional.of(pattern));
+ }
+
+ @Test
+ public void testGetValidLogUrlPatternNoScheme() {
+ testGetValidLogUrlPattern(
+ PATTERN_WITHOUT_SCHEME, Optional.of("http://" + PATTERN_WITHOUT_SCHEME));
+ }
+
+ @Test
+ public void testGetValidLogUrlPatternUnsupportedScheme() {
+ testGetValidLogUrlPattern("file://" + PATTERN_WITHOUT_SCHEME, Optional.empty());
+ }
+
+ @SuppressWarnings("OptionalUsedAsFieldOrParameterType")
+ private void testGetValidLogUrlPattern(
Review Comment:
can be static
##########
flink-runtime-web/src/test/java/org/apache/flink/runtime/webmonitor/utils/LogUrlUtilTest.java:
##########
@@ -0,0 +1,76 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.flink.runtime.webmonitor.utils;
+
+import org.apache.flink.configuration.Configuration;
+
+import org.junit.Test;
+
+import java.util.Optional;
+
+import static org.apache.flink.configuration.HistoryServerOptions.HISTORY_SERVER_JOBMANAGER_LOG_URL_PATTERN;
+import static org.apache.flink.configuration.HistoryServerOptions.HISTORY_SERVER_TASKMANAGER_LOG_URL_PATTERN;
+import static org.assertj.core.api.Assertions.assertThat;
+
+/** Tests for {@link LogUrlUtil}. */
+public class LogUrlUtilTest {
Review Comment:
```suggestion
class LogUrlUtilTest {
```
In JUnit 5, test classes, test methods, and lifecycle methods are not required to be public, but they must not be private. And the [JUnit5 user guide ](https://junit.org/junit5/docs/snapshot/user-guide/#writing-tests-classes-and-methods)recommends omitting the public modifier for them unless there is a technical reason – for example, when a test class is extended by a test class in another package.
--
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@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org