You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@hudi.apache.org by GitBox <gi...@apache.org> on 2020/10/04 03:26:04 UTC
[GitHub] [hudi] lw309637554 opened a new pull request #2142: [HUDI-1203] add port configuration for EmbeddedTimelineService
lw309637554 opened a new pull request #2142:
URL: https://github.com/apache/hudi/pull/2142
## *Tips*
- *Thank you very much for contributing to Apache Hudi.*
- *Please review https://hudi.apache.org/contributing.html before opening a pull request.*
## What is the purpose of the pull request
Allow port configuration for EmbeddedTimelineService, because in some runtime environment such as spark on kubernets need to plan service port . So add hoodie.embed.timeline.server.port for user to set the Embedded port wanted. default is 0 (will generate random port). Also add 16 retries when start server failed.
## Brief change log
*(for example:)*
- *Modify AnnotationLocation checkstyle rule in checkstyle.xml*
## Verify this pull request
*(Please pick either of the following options)*
This pull request is a trivial rework / code cleanup without any test coverage.
*(or)*
This pull request is already covered by existing tests, such as *(please describe tests)*.
(or)
This change added tests and can be verified as follows:
*(example:)*
- *Added integration tests for end-to-end.*
- *Added HoodieClientWriteTest to verify the change.*
- *Manually verified the change by running a job locally.*
## Committer checklist
- [ ] Has a corresponding JIRA in PR title & commit
- [ ] Commit message is descriptive of the change
- [ ] CI is green
- [ ] Necessary doc changes done or have another open PR
- [ ] For large changes, please consider breaking it into sub-tasks under an umbrella JIRA.
----------------------------------------------------------------
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
[GitHub] [hudi] lw309637554 commented on a change in pull request #2142: [HUDI-1203] add port configuration for EmbeddedTimelineService
Posted by GitBox <gi...@apache.org>.
lw309637554 commented on a change in pull request #2142:
URL: https://github.com/apache/hudi/pull/2142#discussion_r499544639
##########
File path: hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/AbstractHoodieClient.java
##########
@@ -102,7 +102,8 @@ private synchronized void startEmbeddedServerView() {
// Run Embedded Timeline Server
LOG.info("Starting Timeline service !!");
Option<String> hostAddr = context.getProperty(EngineProperty.EMBEDDED_SERVER_HOST);
- timelineServer = Option.of(new EmbeddedTimelineService(context, hostAddr.orElse(null),
+ int embeddedTimelineServerPort = config.getEmbeddedTimelineServerPort();
Review comment:
done
----------------------------------------------------------------
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
[GitHub] [hudi] lw309637554 commented on pull request #2142: [HUDI-1203] add port configuration for EmbeddedTimelineService
Posted by GitBox <gi...@apache.org>.
lw309637554 commented on pull request #2142:
URL: https://github.com/apache/hudi/pull/2142#issuecomment-703258454
@bvaradar @leesf please help to review
----------------------------------------------------------------
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
[GitHub] [hudi] lw309637554 commented on pull request #2142: [HUDI-1203] add port configuration for EmbeddedTimelineService
Posted by GitBox <gi...@apache.org>.
lw309637554 commented on pull request #2142:
URL: https://github.com/apache/hudi/pull/2142#issuecomment-703720646
> Minor comments. LGTM otherwise
all the comment have resolved
----------------------------------------------------------------
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
[GitHub] [hudi] lw309637554 commented on a change in pull request #2142: [HUDI-1203] add port configuration for EmbeddedTimelineService
Posted by GitBox <gi...@apache.org>.
lw309637554 commented on a change in pull request #2142:
URL: https://github.com/apache/hudi/pull/2142#discussion_r499545082
##########
File path: hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/embedded/EmbeddedTimelineService.java
##########
@@ -39,17 +39,19 @@
private static final Logger LOG = LogManager.getLogger(EmbeddedTimelineService.class);
private int serverPort;
+ private int userConfigPort;
Review comment:
done
----------------------------------------------------------------
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
[GitHub] [hudi] vinothchandar merged pull request #2142: [HUDI-1203] add port configuration for EmbeddedTimelineService
Posted by GitBox <gi...@apache.org>.
vinothchandar merged pull request #2142:
URL: https://github.com/apache/hudi/pull/2142
----------------------------------------------------------------
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
[GitHub] [hudi] lw309637554 commented on pull request #2142: [HUDI-1203] add port configuration for EmbeddedTimelineService
Posted by GitBox <gi...@apache.org>.
lw309637554 commented on pull request #2142:
URL: https://github.com/apache/hudi/pull/2142#issuecomment-703283495
@vinothchandar can you help to review? thanks.
----------------------------------------------------------------
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
[GitHub] [hudi] lw309637554 commented on a change in pull request #2142: [HUDI-1203] add port configuration for EmbeddedTimelineService
Posted by GitBox <gi...@apache.org>.
lw309637554 commented on a change in pull request #2142:
URL: https://github.com/apache/hudi/pull/2142#discussion_r499548663
##########
File path: hudi-timeline-service/src/main/java/org/apache/hudi/timeline/service/TimelineService.java
##########
@@ -98,16 +98,42 @@ public TimelineService(Config config) throws IOException {
public Boolean help = false;
}
+ private int startServiceOnPort(int port) throws IOException {
+ if (!(port == 0 || (1024 <= port && port < 65536))) {
+ throw new IllegalArgumentException(String.format("startPort should be between 1024 and 65535 (inclusive), "
+ + "or 0 for a random free port. but now is %s.", port));
+ }
+ int maxRetries = 16;
Review comment:
> pull into `static final` variable?
done
----------------------------------------------------------------
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
[GitHub] [hudi] vinothchandar commented on a change in pull request #2142: [HUDI-1203] add port configuration for EmbeddedTimelineService
Posted by GitBox <gi...@apache.org>.
vinothchandar commented on a change in pull request #2142:
URL: https://github.com/apache/hudi/pull/2142#discussion_r499300981
##########
File path: hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/embedded/EmbeddedTimelineService.java
##########
@@ -39,17 +39,19 @@
private static final Logger LOG = LogManager.getLogger(EmbeddedTimelineService.class);
private int serverPort;
+ private int userConfigPort;
Review comment:
rename: preferredPort
##########
File path: hudi-client/hudi-client-common/src/main/java/org/apache/hudi/client/AbstractHoodieClient.java
##########
@@ -102,7 +102,8 @@ private synchronized void startEmbeddedServerView() {
// Run Embedded Timeline Server
LOG.info("Starting Timeline service !!");
Option<String> hostAddr = context.getProperty(EngineProperty.EMBEDDED_SERVER_HOST);
- timelineServer = Option.of(new EmbeddedTimelineService(context, hostAddr.orElse(null),
+ int embeddedTimelineServerPort = config.getEmbeddedTimelineServerPort();
Review comment:
nit: avoid the extra `int` variable?
##########
File path: hudi-timeline-service/src/main/java/org/apache/hudi/timeline/service/TimelineService.java
##########
@@ -98,16 +98,42 @@ public TimelineService(Config config) throws IOException {
public Boolean help = false;
}
+ private int startServiceOnPort(int port) throws IOException {
+ if (!(port == 0 || (1024 <= port && port < 65536))) {
+ throw new IllegalArgumentException(String.format("startPort should be between 1024 and 65535 (inclusive), "
+ + "or 0 for a random free port. but now is %s.", port));
+ }
+ int maxRetries = 16;
Review comment:
you also probably want to add a small wait between attempts?
##########
File path: hudi-timeline-service/src/main/java/org/apache/hudi/timeline/service/TimelineService.java
##########
@@ -98,16 +98,42 @@ public TimelineService(Config config) throws IOException {
public Boolean help = false;
}
+ private int startServiceOnPort(int port) throws IOException {
+ if (!(port == 0 || (1024 <= port && port < 65536))) {
+ throw new IllegalArgumentException(String.format("startPort should be between 1024 and 65535 (inclusive), "
+ + "or 0 for a random free port. but now is %s.", port));
+ }
+ int maxRetries = 16;
+ for (int offset = 0; offset < maxRetries; offset++) {
Review comment:
rename: offset -> attempt?
##########
File path: hudi-timeline-service/src/main/java/org/apache/hudi/timeline/service/TimelineService.java
##########
@@ -98,16 +98,42 @@ public TimelineService(Config config) throws IOException {
public Boolean help = false;
}
+ private int startServiceOnPort(int port) throws IOException {
+ if (!(port == 0 || (1024 <= port && port < 65536))) {
+ throw new IllegalArgumentException(String.format("startPort should be between 1024 and 65535 (inclusive), "
+ + "or 0 for a random free port. but now is %s.", port));
+ }
+ int maxRetries = 16;
Review comment:
pull into `static final` variable?
##########
File path: hudi-timeline-service/src/main/java/org/apache/hudi/timeline/service/TimelineService.java
##########
@@ -98,16 +98,42 @@ public TimelineService(Config config) throws IOException {
public Boolean help = false;
}
+ private int startServiceOnPort(int port) throws IOException {
+ if (!(port == 0 || (1024 <= port && port < 65536))) {
+ throw new IllegalArgumentException(String.format("startPort should be between 1024 and 65535 (inclusive), "
+ + "or 0 for a random free port. but now is %s.", port));
+ }
+ int maxRetries = 16;
+ for (int offset = 0; offset < maxRetries; offset++) {
+ int tryPort = port == 0 ? port : (port + offset - 1024) % (65536 - 1024) + 1024;
Review comment:
what are we trying to do in the `:` else part? a line of comment could help
##########
File path: hudi-timeline-service/src/main/java/org/apache/hudi/timeline/service/TimelineService.java
##########
@@ -98,16 +98,42 @@ public TimelineService(Config config) throws IOException {
public Boolean help = false;
}
+ private int startServiceOnPort(int port) throws IOException {
+ if (!(port == 0 || (1024 <= port && port < 65536))) {
+ throw new IllegalArgumentException(String.format("startPort should be between 1024 and 65535 (inclusive), "
+ + "or 0 for a random free port. but now is %s.", port));
+ }
+ int maxRetries = 16;
+ for (int offset = 0; offset < maxRetries; offset++) {
+ int tryPort = port == 0 ? port : (port + offset - 1024) % (65536 - 1024) + 1024;
+ try {
+ app.start(tryPort);
+ return app.port();
+ } catch (Exception e) {
+ if (e.getMessage() != null && e.getMessage().contains("Failed to bind to")) {
+ if (tryPort == 0) {
+ LOG.warn("Timeline server could not bind on a random free port.");
+ } else {
+ LOG.warn(String.format("Timeline server could not bind on port %d. "
+ + "Attempting port %d + 1.",tryPort, tryPort));
+ }
+ } else {
+ LOG.warn(String.format("Timeline server start failed on port %d. Attempting port %d + 1.",tryPort, tryPort), e);
+ }
+ }
+ }
+ throw new IOException(String.format("Timeline server start failed on port %d, after retry %d time", port, maxRetries));
Review comment:
nit: %d times
----------------------------------------------------------------
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
[GitHub] [hudi] lw309637554 commented on a change in pull request #2142: [HUDI-1203] add port configuration for EmbeddedTimelineService
Posted by GitBox <gi...@apache.org>.
lw309637554 commented on a change in pull request #2142:
URL: https://github.com/apache/hudi/pull/2142#discussion_r499549836
##########
File path: hudi-timeline-service/src/main/java/org/apache/hudi/timeline/service/TimelineService.java
##########
@@ -98,16 +98,42 @@ public TimelineService(Config config) throws IOException {
public Boolean help = false;
}
+ private int startServiceOnPort(int port) throws IOException {
+ if (!(port == 0 || (1024 <= port && port < 65536))) {
+ throw new IllegalArgumentException(String.format("startPort should be between 1024 and 65535 (inclusive), "
+ + "or 0 for a random free port. but now is %s.", port));
+ }
+ int maxRetries = 16;
Review comment:
> you also probably want to add a small wait between attempts?
i think not need to add a small wait between attempts. because in most cases will retry when port is already in use, a attempt will change to new port .
##########
File path: hudi-timeline-service/src/main/java/org/apache/hudi/timeline/service/TimelineService.java
##########
@@ -98,16 +98,42 @@ public TimelineService(Config config) throws IOException {
public Boolean help = false;
}
+ private int startServiceOnPort(int port) throws IOException {
+ if (!(port == 0 || (1024 <= port && port < 65536))) {
+ throw new IllegalArgumentException(String.format("startPort should be between 1024 and 65535 (inclusive), "
+ + "or 0 for a random free port. but now is %s.", port));
+ }
+ int maxRetries = 16;
+ for (int offset = 0; offset < maxRetries; offset++) {
Review comment:
done
----------------------------------------------------------------
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
[GitHub] [hudi] lw309637554 commented on a change in pull request #2142: [HUDI-1203] add port configuration for EmbeddedTimelineService
Posted by GitBox <gi...@apache.org>.
lw309637554 commented on a change in pull request #2142:
URL: https://github.com/apache/hudi/pull/2142#discussion_r499550041
##########
File path: hudi-timeline-service/src/main/java/org/apache/hudi/timeline/service/TimelineService.java
##########
@@ -98,16 +98,42 @@ public TimelineService(Config config) throws IOException {
public Boolean help = false;
}
+ private int startServiceOnPort(int port) throws IOException {
+ if (!(port == 0 || (1024 <= port && port < 65536))) {
+ throw new IllegalArgumentException(String.format("startPort should be between 1024 and 65535 (inclusive), "
+ + "or 0 for a random free port. but now is %s.", port));
+ }
+ int maxRetries = 16;
+ for (int offset = 0; offset < maxRetries; offset++) {
+ int tryPort = port == 0 ? port : (port + offset - 1024) % (65536 - 1024) + 1024;
Review comment:
done
##########
File path: hudi-timeline-service/src/main/java/org/apache/hudi/timeline/service/TimelineService.java
##########
@@ -98,16 +98,42 @@ public TimelineService(Config config) throws IOException {
public Boolean help = false;
}
+ private int startServiceOnPort(int port) throws IOException {
+ if (!(port == 0 || (1024 <= port && port < 65536))) {
+ throw new IllegalArgumentException(String.format("startPort should be between 1024 and 65535 (inclusive), "
+ + "or 0 for a random free port. but now is %s.", port));
+ }
+ int maxRetries = 16;
+ for (int offset = 0; offset < maxRetries; offset++) {
+ int tryPort = port == 0 ? port : (port + offset - 1024) % (65536 - 1024) + 1024;
+ try {
+ app.start(tryPort);
+ return app.port();
+ } catch (Exception e) {
+ if (e.getMessage() != null && e.getMessage().contains("Failed to bind to")) {
+ if (tryPort == 0) {
+ LOG.warn("Timeline server could not bind on a random free port.");
+ } else {
+ LOG.warn(String.format("Timeline server could not bind on port %d. "
+ + "Attempting port %d + 1.",tryPort, tryPort));
+ }
+ } else {
+ LOG.warn(String.format("Timeline server start failed on port %d. Attempting port %d + 1.",tryPort, tryPort), e);
+ }
+ }
+ }
+ throw new IOException(String.format("Timeline server start failed on port %d, after retry %d time", port, maxRetries));
Review comment:
done
----------------------------------------------------------------
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