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 2020/10/20 19:30:55 UTC
[GitHub] [flink] rmetzger commented on a change in pull request #13706: [FLINK-19677][runtime] Make JobManager lazily resolve hostname of TaskManager and provide an option to turn off reverse resolution entirely
rmetzger commented on a change in pull request #13706:
URL: https://github.com/apache/flink/pull/13706#discussion_r508697760
##########
File path: flink-core/src/main/java/org/apache/flink/configuration/JobManagerOptions.java
##########
@@ -308,6 +308,19 @@
.defaultValue(Integer.MAX_VALUE)
.withDescription("The max number of completed jobs that can be kept in the job store.");
+ /**
+ * Flag indicating whether JobManager would retrieve canonical host name of TaskManager during registration.
+ */
+ @Documentation.Section(Documentation.Sections.EXPERT_SCHEDULING)
Review comment:
I don't think this config option is related to scheduling.
##########
File path: flink-runtime/src/main/java/org/apache/flink/runtime/taskmanager/TaskManagerLocation.java
##########
@@ -150,28 +166,48 @@ public String addressString() {
/**
* Returns the fully-qualified domain name the TaskManager. If the name could not be
- * determined, the return value will be a textual representation of the TaskManager's IP address.
+ * determined or {@link JobManagerOptions.RETRIEVE_TASK_MANAGER_HOSTNAME} is set to false,
+ * the return value will be a textual representation of the TaskManager's IP address.
*
* @return The fully-qualified domain name of the TaskManager.
*/
public String getFQDNHostname() {
+ // Lazily retrieve FQDN host name of this TaskManager
+ if (fqdnHostName == null) {
+ if (!retrieveHostName) {
+ fqdnHostName = inetAddress.getHostAddress();
+ } else {
+ fqdnHostName = getFqdnHostName(inetAddress);
+ }
+ }
+
return fqdnHostName;
}
/**
- * Gets the hostname of the TaskManager. The hostname derives from the fully qualified
- * domain name (FQDN, see {@link #getFQDNHostname()}):
+ * Gets the hostname of the TaskManager.
+ * If {@link JobManagerOptions.RETRIEVE_TASK_MANAGER_HOSTNAME} is true,
Review comment:
again, this class is exposing an implementation detail of `JobMaster`.
(I'm a bit uncertain about this review item, maybe it is also okay. But I believe it is dangerous to mention this here as the implementation could change)
##########
File path: flink-runtime/src/main/java/org/apache/flink/runtime/jobmaster/JobMaster.java
##########
@@ -606,7 +608,12 @@ private void releaseEmptyTaskManager(ResourceID resourceId) {
final TaskManagerLocation taskManagerLocation;
try {
- taskManagerLocation = TaskManagerLocation.fromUnresolvedLocation(unresolvedTaskManagerLocation);
+ final boolean retrieveTaskManagerHostName = this.jobMasterConfiguration.getConfiguration()
+ .getBoolean(JobManagerOptions.RETRIEVE_TASK_MANAGER_HOSTNAME);
+ if (!retrieveTaskManagerHostName) {
+ log.info("JobManager would not retrieve TaskManager's canonical hostname due to configuration.");
Review comment:
I don't think it is a good idea to log this message every time we register a taskmanager.
On startup, we are logging all configuration parameters. I think that is enough in this case OR we log on debug in the constructor when looking up the config key?
##########
File path: flink-runtime/src/main/java/org/apache/flink/runtime/taskmanager/TaskManagerLocation.java
##########
@@ -150,28 +166,48 @@ public String addressString() {
/**
* Returns the fully-qualified domain name the TaskManager. If the name could not be
- * determined, the return value will be a textual representation of the TaskManager's IP address.
+ * determined or {@link JobManagerOptions.RETRIEVE_TASK_MANAGER_HOSTNAME} is set to false,
Review comment:
Isn't the use of this configuration option an implementation of the `JobMaster`, not visible to this class?
I would only mention the `retrieveHostName` field here.
##########
File path: flink-core/src/main/java/org/apache/flink/configuration/JobManagerOptions.java
##########
@@ -308,6 +308,19 @@
.defaultValue(Integer.MAX_VALUE)
.withDescription("The max number of completed jobs that can be kept in the job store.");
+ /**
+ * Flag indicating whether JobManager would retrieve canonical host name of TaskManager during registration.
+ */
+ @Documentation.Section(Documentation.Sections.EXPERT_SCHEDULING)
Review comment:
```suggestion
@Documentation.Section(Documentation.Sections.ALL_JOB_MANAGER)
```
##########
File path: flink-runtime/src/test/java/org/apache/flink/runtime/taskmanager/TaskManagerLocationTest.java
##########
@@ -198,4 +199,24 @@ public void testGetHostname2() {
fail(e.getMessage());
}
}
+
+ @Test
+ public void testNotRetrieveHostName() {
+ try {
+ InetAddress address = mock(InetAddress.class);
+ when(address.getCanonicalHostName()).thenReturn("worker10");
+ when(address.getHostName()).thenReturn("worker10");
+ when(address.getHostAddress()).thenReturn("127.0.0.1");
+
+ TaskManagerLocation info = new TaskManagerLocation(ResourceID.generate(), address, 19871, false);
+ assertNotEquals("worker10", info.getHostname());
+ assertNotEquals("worker10", info.getFQDNHostname());
+ assertEquals("127.0.0.1", info.getHostname());
+ assertEquals("127.0.0.1", info.getFQDNHostname());
+ }
+ catch (Exception e) {
+ e.printStackTrace();
+ fail(e.getMessage());
Review comment:
You don't need to manually fail the test. if there's an exception thrown out of the test, it'll fail.
The other code in this file is very old, and uses an outdated pattern 😄
##########
File path: flink-runtime/src/main/java/org/apache/flink/runtime/jobmaster/JobMaster.java
##########
@@ -606,7 +608,12 @@ private void releaseEmptyTaskManager(ResourceID resourceId) {
final TaskManagerLocation taskManagerLocation;
try {
- taskManagerLocation = TaskManagerLocation.fromUnresolvedLocation(unresolvedTaskManagerLocation);
+ final boolean retrieveTaskManagerHostName = this.jobMasterConfiguration.getConfiguration()
+ .getBoolean(JobManagerOptions.RETRIEVE_TASK_MANAGER_HOSTNAME);
Review comment:
I was wondering whether it makes sense to move this local variable to a field, and do this lookup in the constructor only once?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org