You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@gobblin.apache.org by GitBox <gi...@apache.org> on 2022/11/18 00:07:52 UTC
[GitHub] [gobblin] umustafi opened a new pull request, #3602: [GOBBLIN-1743] Ensure GobblinTaskRunner works without Yarn use
umustafi opened a new pull request, #3602:
URL: https://github.com/apache/gobblin/pull/3602
Dear Gobblin maintainers,
Please accept this PR. I understand that it will not be reviewed until I have checked off all the steps below!
### JIRA
- [X] My PR addresses the following [Gobblin JIRA](https://issues.apache.org/jira/browse/GOBBLIN/) issues and references them in the PR title. For example, "[GOBBLIN-XXX] My Gobblin PR"
- https://issues.apache.org/jira/browse/GOBBLIN-1743
### Description
- [X] Here are some details about my PR, including screenshots (if applicable):
this PR https://github.com/apache/gobblin/pull/3519/files has a call to Helix cluster contained outside of a conditional that was previously never encountered for use cases without yarn. I moved the call back inside the conditional to ensure we don't attempt to retrieve an instanceConfig that is not registered with Helix as we don't have Yarn mode enabled.
### Tests
- [X] My PR adds the following unit tests __OR__ does not need testing for this extremely good reason:
### Commits
- [X] My commits all reference JIRA issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "[How to write a good git commit message](http://chris.beams.io/posts/git-commit/)":
1. Subject is separated from body by a blank line
2. Subject is limited to 50 characters
3. Subject does not end with a period
4. Subject uses the imperative mood ("add", not "adding")
5. Body wraps at 72 characters
6. Body explains "what" and "why", not "how"
--
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: dev-unsubscribe@gobblin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [gobblin] arjun4084346 commented on a diff in pull request #3602: [GOBBLIN-1743] Ensure GobblinTaskRunner works without Yarn use
Posted by GitBox <gi...@apache.org>.
arjun4084346 commented on code in PR #3602:
URL: https://github.com/apache/gobblin/pull/3602#discussion_r1025878463
##########
gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinTaskRunner.java:
##########
@@ -544,23 +544,24 @@ void connectHelixManagerWithRetry() {
private void addInstanceTags() {
HelixManager receiverManager = getReceiverManager();
if (receiverManager.isConnected()) {
- // The helix instance associated with this container should be consistent on helix tag
- List<String> existedTags = receiverManager.getClusterManagmentTool()
- .getInstanceConfig(this.clusterName, this.helixInstanceName).getTags();
- Set<String> desiredTags = new HashSet<>(
- ConfigUtils.getStringList(this.clusterConfig, GobblinClusterConfigurationKeys.HELIX_INSTANCE_TAGS_KEY));
- if (!desiredTags.isEmpty()) {
- // Remove tag assignments for the current Helix instance from a previous run
- for (String tag : existedTags) {
- if (!desiredTags.contains(tag))
- receiverManager.getClusterManagmentTool().removeInstanceTag(this.clusterName, this.helixInstanceName, tag);
- logger.info("Removed unrelated helix tag {} for instance {}", tag, this.helixInstanceName);
+ try {
+ Set<String> desiredTags = new HashSet<>(ConfigUtils.getStringList(this.clusterConfig, GobblinClusterConfigurationKeys.HELIX_INSTANCE_TAGS_KEY));
+ if (!desiredTags.isEmpty()) {
+ // The helix instance associated with this container should be consistent on helix tag
+ List<String> existedTags =
+ receiverManager.getClusterManagmentTool().getInstanceConfig(this.clusterName, this.helixInstanceName).getTags();
+ // Remove tag assignments for the current Helix instance from a previous run
+ for (String tag : existedTags) {
+ if (!desiredTags.contains(tag))
+ receiverManager.getClusterManagmentTool().removeInstanceTag(this.clusterName, this.helixInstanceName, tag);
Review Comment:
Oh, maybe the log line should also go inside `{}`
--
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: dev-unsubscribe@gobblin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [gobblin] arjun4084346 commented on a diff in pull request #3602: [GOBBLIN-1743] Ensure GobblinTaskRunner works without Yarn use
Posted by GitBox <gi...@apache.org>.
arjun4084346 commented on code in PR #3602:
URL: https://github.com/apache/gobblin/pull/3602#discussion_r1025878094
##########
gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinTaskRunner.java:
##########
@@ -544,23 +544,24 @@ void connectHelixManagerWithRetry() {
private void addInstanceTags() {
HelixManager receiverManager = getReceiverManager();
if (receiverManager.isConnected()) {
- // The helix instance associated with this container should be consistent on helix tag
- List<String> existedTags = receiverManager.getClusterManagmentTool()
- .getInstanceConfig(this.clusterName, this.helixInstanceName).getTags();
- Set<String> desiredTags = new HashSet<>(
- ConfigUtils.getStringList(this.clusterConfig, GobblinClusterConfigurationKeys.HELIX_INSTANCE_TAGS_KEY));
- if (!desiredTags.isEmpty()) {
- // Remove tag assignments for the current Helix instance from a previous run
- for (String tag : existedTags) {
- if (!desiredTags.contains(tag))
- receiverManager.getClusterManagmentTool().removeInstanceTag(this.clusterName, this.helixInstanceName, tag);
- logger.info("Removed unrelated helix tag {} for instance {}", tag, this.helixInstanceName);
+ try {
+ Set<String> desiredTags = new HashSet<>(ConfigUtils.getStringList(this.clusterConfig, GobblinClusterConfigurationKeys.HELIX_INSTANCE_TAGS_KEY));
+ if (!desiredTags.isEmpty()) {
+ // The helix instance associated with this container should be consistent on helix tag
+ List<String> existedTags =
+ receiverManager.getClusterManagmentTool().getInstanceConfig(this.clusterName, this.helixInstanceName).getTags();
+ // Remove tag assignments for the current Helix instance from a previous run
+ for (String tag : existedTags) {
+ if (!desiredTags.contains(tag))
+ receiverManager.getClusterManagmentTool().removeInstanceTag(this.clusterName, this.helixInstanceName, tag);
Review Comment:
another nit, lets put this one line inside `{}`
Is that not already the part of codestyle?
--
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: dev-unsubscribe@gobblin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [gobblin] umustafi commented on a diff in pull request #3602: [GOBBLIN-1743] Ensure GobblinTaskRunner works without Yarn use
Posted by GitBox <gi...@apache.org>.
umustafi commented on code in PR #3602:
URL: https://github.com/apache/gobblin/pull/3602#discussion_r1026981774
##########
gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinTaskRunner.java:
##########
@@ -599,6 +601,7 @@ public void run() {
private Optional<ContainerMetrics> buildContainerMetrics() {
Properties properties = ConfigUtils.configToProperties(this.clusterConfig);
if (GobblinMetrics.isEnabled(properties)) {
+ logger.info("Container metrics are enabled");
Review Comment:
The `METRICS_KAFKA_TOPIC` and other Kafka related metrics parameters in `ConfigurationKeys` are what we use to define where the `ScheduledReporter` outputs metrics.
--
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: dev-unsubscribe@gobblin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [gobblin] umustafi commented on a diff in pull request #3602: [GOBBLIN-1743] Ensure GobblinTaskRunner works without Yarn use
Posted by GitBox <gi...@apache.org>.
umustafi commented on code in PR #3602:
URL: https://github.com/apache/gobblin/pull/3602#discussion_r1026980719
##########
gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinTaskRunner.java:
##########
@@ -544,23 +544,25 @@ void connectHelixManagerWithRetry() {
private void addInstanceTags() {
HelixManager receiverManager = getReceiverManager();
if (receiverManager.isConnected()) {
- // The helix instance associated with this container should be consistent on helix tag
- List<String> existedTags = receiverManager.getClusterManagmentTool()
- .getInstanceConfig(this.clusterName, this.helixInstanceName).getTags();
- Set<String> desiredTags = new HashSet<>(
- ConfigUtils.getStringList(this.clusterConfig, GobblinClusterConfigurationKeys.HELIX_INSTANCE_TAGS_KEY));
- if (!desiredTags.isEmpty()) {
- // Remove tag assignments for the current Helix instance from a previous run
- for (String tag : existedTags) {
- if (!desiredTags.contains(tag))
- receiverManager.getClusterManagmentTool().removeInstanceTag(this.clusterName, this.helixInstanceName, tag);
- logger.info("Removed unrelated helix tag {} for instance {}", tag, this.helixInstanceName);
+ try {
+ Set<String> desiredTags = new HashSet<>(ConfigUtils.getStringList(this.clusterConfig, GobblinClusterConfigurationKeys.HELIX_INSTANCE_TAGS_KEY));
+ if (!desiredTags.isEmpty()) {
Review Comment:
Yes that's a good point, if it is yarn specific it should lie in the `GobblinYarnTaskRunner` perhaps. However, we _can_ technically run gobblin-cluster in _non-standalone_ mode and which would rely on this code. Afaik we don't actually use that and best practice may be to use the child class instead however I don't want to change anything too drastically for the current fix.
--
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: dev-unsubscribe@gobblin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [gobblin] hanghangliu commented on pull request #3602: [GOBBLIN-1743] Ensure GobblinTaskRunner works without Yarn use
Posted by GitBox <gi...@apache.org>.
hanghangliu commented on PR #3602:
URL: https://github.com/apache/gobblin/pull/3602#issuecomment-1319451567
LGTM. +1
--
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: dev-unsubscribe@gobblin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [gobblin] umustafi commented on a diff in pull request #3602: [GOBBLIN-1743] Ensure GobblinTaskRunner works without Yarn use
Posted by GitBox <gi...@apache.org>.
umustafi commented on code in PR #3602:
URL: https://github.com/apache/gobblin/pull/3602#discussion_r1025879976
##########
gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinTaskRunner.java:
##########
@@ -544,23 +544,24 @@ void connectHelixManagerWithRetry() {
private void addInstanceTags() {
HelixManager receiverManager = getReceiverManager();
if (receiverManager.isConnected()) {
- // The helix instance associated with this container should be consistent on helix tag
- List<String> existedTags = receiverManager.getClusterManagmentTool()
- .getInstanceConfig(this.clusterName, this.helixInstanceName).getTags();
- Set<String> desiredTags = new HashSet<>(
- ConfigUtils.getStringList(this.clusterConfig, GobblinClusterConfigurationKeys.HELIX_INSTANCE_TAGS_KEY));
- if (!desiredTags.isEmpty()) {
- // Remove tag assignments for the current Helix instance from a previous run
- for (String tag : existedTags) {
- if (!desiredTags.contains(tag))
- receiverManager.getClusterManagmentTool().removeInstanceTag(this.clusterName, this.helixInstanceName, tag);
- logger.info("Removed unrelated helix tag {} for instance {}", tag, this.helixInstanceName);
+ try {
+ Set<String> desiredTags = new HashSet<>(ConfigUtils.getStringList(this.clusterConfig, GobblinClusterConfigurationKeys.HELIX_INSTANCE_TAGS_KEY));
+ if (!desiredTags.isEmpty()) {
+ // The helix instance associated with this container should be consistent on helix tag
+ List<String> existedTags =
+ receiverManager.getClusterManagmentTool().getInstanceConfig(this.clusterName, this.helixInstanceName).getTags();
+ // Remove tag assignments for the current Helix instance from a previous run
+ for (String tag : existedTags) {
+ if (!desiredTags.contains(tag))
+ receiverManager.getClusterManagmentTool().removeInstanceTag(this.clusterName, this.helixInstanceName, tag);
Review Comment:
Yea I also thought this was strange, adding.
--
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: dev-unsubscribe@gobblin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [gobblin] umustafi commented on a diff in pull request #3602: [GOBBLIN-1743] Ensure GobblinTaskRunner works without Yarn use
Posted by GitBox <gi...@apache.org>.
umustafi commented on code in PR #3602:
URL: https://github.com/apache/gobblin/pull/3602#discussion_r1028390996
##########
gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinTaskRunner.java:
##########
@@ -544,23 +544,25 @@ void connectHelixManagerWithRetry() {
private void addInstanceTags() {
HelixManager receiverManager = getReceiverManager();
if (receiverManager.isConnected()) {
- // The helix instance associated with this container should be consistent on helix tag
- List<String> existedTags = receiverManager.getClusterManagmentTool()
- .getInstanceConfig(this.clusterName, this.helixInstanceName).getTags();
- Set<String> desiredTags = new HashSet<>(
- ConfigUtils.getStringList(this.clusterConfig, GobblinClusterConfigurationKeys.HELIX_INSTANCE_TAGS_KEY));
- if (!desiredTags.isEmpty()) {
- // Remove tag assignments for the current Helix instance from a previous run
- for (String tag : existedTags) {
- if (!desiredTags.contains(tag))
- receiverManager.getClusterManagmentTool().removeInstanceTag(this.clusterName, this.helixInstanceName, tag);
- logger.info("Removed unrelated helix tag {} for instance {}", tag, this.helixInstanceName);
+ try {
+ Set<String> desiredTags = new HashSet<>(ConfigUtils.getStringList(this.clusterConfig, GobblinClusterConfigurationKeys.HELIX_INSTANCE_TAGS_KEY));
+ if (!desiredTags.isEmpty()) {
Review Comment:
Standalone mode = no yarn. Yes, non standalone does use Yarn. From what I can see in the code it changes how the [GobblinHelixMultiManager](https://jarvis.corp.linkedin.com/codesearch/result/?name=GobblinHelixMultiManager.java&path=gobblin-elr%2Fgobblin-cluster%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fgobblin%2Fcluster&reponame=linkedin%2Fgobblin-elr#275) and more importantly [GobblinClusterManager](https://jarvis.corp.linkedin.com/codesearch/result/?name=GobblinClusterManager.java&path=gobblin-elr%2Fgobblin-cluster%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fgobblin%2Fcluster&reponame=linkedin%2Fgobblin-elr#286) work. I have to dig around more to see how `GobblinYarnTaskRunner` is used. Let's investigate offline.
--
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: dev-unsubscribe@gobblin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [gobblin] homatthew commented on a diff in pull request #3602: [GOBBLIN-1743] Ensure GobblinTaskRunner works without Yarn use
Posted by GitBox <gi...@apache.org>.
homatthew commented on code in PR #3602:
URL: https://github.com/apache/gobblin/pull/3602#discussion_r1025937082
##########
gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinTaskRunner.java:
##########
@@ -544,23 +544,25 @@ void connectHelixManagerWithRetry() {
private void addInstanceTags() {
HelixManager receiverManager = getReceiverManager();
if (receiverManager.isConnected()) {
- // The helix instance associated with this container should be consistent on helix tag
- List<String> existedTags = receiverManager.getClusterManagmentTool()
- .getInstanceConfig(this.clusterName, this.helixInstanceName).getTags();
- Set<String> desiredTags = new HashSet<>(
- ConfigUtils.getStringList(this.clusterConfig, GobblinClusterConfigurationKeys.HELIX_INSTANCE_TAGS_KEY));
- if (!desiredTags.isEmpty()) {
- // Remove tag assignments for the current Helix instance from a previous run
- for (String tag : existedTags) {
- if (!desiredTags.contains(tag))
- receiverManager.getClusterManagmentTool().removeInstanceTag(this.clusterName, this.helixInstanceName, tag);
- logger.info("Removed unrelated helix tag {} for instance {}", tag, this.helixInstanceName);
+ try {
+ Set<String> desiredTags = new HashSet<>(ConfigUtils.getStringList(this.clusterConfig, GobblinClusterConfigurationKeys.HELIX_INSTANCE_TAGS_KEY));
Review Comment:
Nit: you removed the new line after new `HashSet<>(`. If possible, can we keep the whitespace the same to keep the git history cleaner?
##########
gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinTaskRunner.java:
##########
@@ -544,23 +544,25 @@ void connectHelixManagerWithRetry() {
private void addInstanceTags() {
HelixManager receiverManager = getReceiverManager();
if (receiverManager.isConnected()) {
- // The helix instance associated with this container should be consistent on helix tag
- List<String> existedTags = receiverManager.getClusterManagmentTool()
- .getInstanceConfig(this.clusterName, this.helixInstanceName).getTags();
- Set<String> desiredTags = new HashSet<>(
- ConfigUtils.getStringList(this.clusterConfig, GobblinClusterConfigurationKeys.HELIX_INSTANCE_TAGS_KEY));
- if (!desiredTags.isEmpty()) {
- // Remove tag assignments for the current Helix instance from a previous run
- for (String tag : existedTags) {
- if (!desiredTags.contains(tag))
- receiverManager.getClusterManagmentTool().removeInstanceTag(this.clusterName, this.helixInstanceName, tag);
- logger.info("Removed unrelated helix tag {} for instance {}", tag, this.helixInstanceName);
+ try {
+ Set<String> desiredTags = new HashSet<>(ConfigUtils.getStringList(this.clusterConfig, GobblinClusterConfigurationKeys.HELIX_INSTANCE_TAGS_KEY));
+ if (!desiredTags.isEmpty()) {
Review Comment:
From what I can tell, the reason this works is because the `GobblinYarnTaskRunner` is the only one to set the config key for `HELIX_INSTANCE_TAGS_KEY`. And so by proxy we know that if this desired tags is empty, then we can skip this code because this is not being run by the `GobblinYarnTaskRunner`. But this all feels pretty messy and a bad experience for anyone in the future working on this.
Can we find a better approach with an override method? Isn't the real code smell that this code is inherently coupled with the YarnService / YarnAutoScalingManager? But since this code is being used outside the yarn context, it should instead live inside the `GobblinYarnTaskRunner` somehow.
Admittedly, I am not that big of a fan of inheritance, but since we are already using it in this class it makes sense to use that approach
```
public class GobblinYarnTaskRunner extends GobblinTaskRunner
```
##########
gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinTaskRunner.java:
##########
@@ -544,23 +544,25 @@ void connectHelixManagerWithRetry() {
private void addInstanceTags() {
HelixManager receiverManager = getReceiverManager();
if (receiverManager.isConnected()) {
- // The helix instance associated with this container should be consistent on helix tag
- List<String> existedTags = receiverManager.getClusterManagmentTool()
- .getInstanceConfig(this.clusterName, this.helixInstanceName).getTags();
- Set<String> desiredTags = new HashSet<>(
- ConfigUtils.getStringList(this.clusterConfig, GobblinClusterConfigurationKeys.HELIX_INSTANCE_TAGS_KEY));
- if (!desiredTags.isEmpty()) {
- // Remove tag assignments for the current Helix instance from a previous run
- for (String tag : existedTags) {
- if (!desiredTags.contains(tag))
- receiverManager.getClusterManagmentTool().removeInstanceTag(this.clusterName, this.helixInstanceName, tag);
- logger.info("Removed unrelated helix tag {} for instance {}", tag, this.helixInstanceName);
+ try {
+ Set<String> desiredTags = new HashSet<>(ConfigUtils.getStringList(this.clusterConfig, GobblinClusterConfigurationKeys.HELIX_INSTANCE_TAGS_KEY));
+ if (!desiredTags.isEmpty()) {
+ // The helix instance associated with this container should be consistent on helix tag
+ List<String> existedTags =
+ receiverManager.getClusterManagmentTool().getInstanceConfig(this.clusterName, this.helixInstanceName).getTags();
Review Comment:
It seems to me that this would return empty (but clearly it doesn't since it causes an exception).
##########
gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinTaskRunner.java:
##########
@@ -544,23 +544,25 @@ void connectHelixManagerWithRetry() {
private void addInstanceTags() {
HelixManager receiverManager = getReceiverManager();
if (receiverManager.isConnected()) {
- // The helix instance associated with this container should be consistent on helix tag
- List<String> existedTags = receiverManager.getClusterManagmentTool()
- .getInstanceConfig(this.clusterName, this.helixInstanceName).getTags();
- Set<String> desiredTags = new HashSet<>(
- ConfigUtils.getStringList(this.clusterConfig, GobblinClusterConfigurationKeys.HELIX_INSTANCE_TAGS_KEY));
- if (!desiredTags.isEmpty()) {
- // Remove tag assignments for the current Helix instance from a previous run
- for (String tag : existedTags) {
- if (!desiredTags.contains(tag))
- receiverManager.getClusterManagmentTool().removeInstanceTag(this.clusterName, this.helixInstanceName, tag);
- logger.info("Removed unrelated helix tag {} for instance {}", tag, this.helixInstanceName);
+ try {
+ Set<String> desiredTags = new HashSet<>(ConfigUtils.getStringList(this.clusterConfig, GobblinClusterConfigurationKeys.HELIX_INSTANCE_TAGS_KEY));
Review Comment:
Ditto for the other whitespace type changes
##########
gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinTaskRunner.java:
##########
@@ -599,6 +601,7 @@ public void run() {
private Optional<ContainerMetrics> buildContainerMetrics() {
Properties properties = ConfigUtils.configToProperties(this.clusterConfig);
if (GobblinMetrics.isEnabled(properties)) {
+ logger.info("Container metrics are enabled");
Review Comment:
Nice log addition! Do you know where these container metrics are collected?
##########
gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinTaskRunner.java:
##########
@@ -544,23 +544,25 @@ void connectHelixManagerWithRetry() {
private void addInstanceTags() {
HelixManager receiverManager = getReceiverManager();
if (receiverManager.isConnected()) {
- // The helix instance associated with this container should be consistent on helix tag
- List<String> existedTags = receiverManager.getClusterManagmentTool()
- .getInstanceConfig(this.clusterName, this.helixInstanceName).getTags();
- Set<String> desiredTags = new HashSet<>(
- ConfigUtils.getStringList(this.clusterConfig, GobblinClusterConfigurationKeys.HELIX_INSTANCE_TAGS_KEY));
- if (!desiredTags.isEmpty()) {
- // Remove tag assignments for the current Helix instance from a previous run
- for (String tag : existedTags) {
- if (!desiredTags.contains(tag))
- receiverManager.getClusterManagmentTool().removeInstanceTag(this.clusterName, this.helixInstanceName, tag);
- logger.info("Removed unrelated helix tag {} for instance {}", tag, this.helixInstanceName);
+ try {
+ Set<String> desiredTags = new HashSet<>(ConfigUtils.getStringList(this.clusterConfig, GobblinClusterConfigurationKeys.HELIX_INSTANCE_TAGS_KEY));
+ if (!desiredTags.isEmpty()) {
+ // The helix instance associated with this container should be consistent on helix tag
+ List<String> existedTags =
+ receiverManager.getClusterManagmentTool().getInstanceConfig(this.clusterName, this.helixInstanceName).getTags();
Review Comment:
Why does moving this line here address the issue?
In what situations does a config not exist for an instance? Seems to me like the sequence of events is:
```
on start(), connectHelixManager() makes taskrunner join as a participant
Helix should make an instance config for the taskrunner
```
In what scenario does the above not happen?
--
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: dev-unsubscribe@gobblin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [gobblin] umustafi commented on a diff in pull request #3602: [GOBBLIN-1743] Ensure GobblinTaskRunner works without Yarn use
Posted by GitBox <gi...@apache.org>.
umustafi commented on code in PR #3602:
URL: https://github.com/apache/gobblin/pull/3602#discussion_r1026981774
##########
gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinTaskRunner.java:
##########
@@ -599,6 +601,7 @@ public void run() {
private Optional<ContainerMetrics> buildContainerMetrics() {
Properties properties = ConfigUtils.configToProperties(this.clusterConfig);
if (GobblinMetrics.isEnabled(properties)) {
+ logger.info("Container metrics are enabled");
Review Comment:
The `METRICS_KAFKA_TOPIC` and other Kafka related metrics parameters in `ConfigurationKeys` are what we use to define where the `ScheduledReporter` outputs metrics.
--
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: dev-unsubscribe@gobblin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [gobblin] umustafi commented on a diff in pull request #3602: [GOBBLIN-1743] Ensure GobblinTaskRunner works without Yarn use
Posted by GitBox <gi...@apache.org>.
umustafi commented on code in PR #3602:
URL: https://github.com/apache/gobblin/pull/3602#discussion_r1026979650
##########
gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinTaskRunner.java:
##########
@@ -544,23 +544,25 @@ void connectHelixManagerWithRetry() {
private void addInstanceTags() {
HelixManager receiverManager = getReceiverManager();
if (receiverManager.isConnected()) {
- // The helix instance associated with this container should be consistent on helix tag
- List<String> existedTags = receiverManager.getClusterManagmentTool()
- .getInstanceConfig(this.clusterName, this.helixInstanceName).getTags();
- Set<String> desiredTags = new HashSet<>(
- ConfigUtils.getStringList(this.clusterConfig, GobblinClusterConfigurationKeys.HELIX_INSTANCE_TAGS_KEY));
- if (!desiredTags.isEmpty()) {
- // Remove tag assignments for the current Helix instance from a previous run
- for (String tag : existedTags) {
- if (!desiredTags.contains(tag))
- receiverManager.getClusterManagmentTool().removeInstanceTag(this.clusterName, this.helixInstanceName, tag);
- logger.info("Removed unrelated helix tag {} for instance {}", tag, this.helixInstanceName);
+ try {
+ Set<String> desiredTags = new HashSet<>(ConfigUtils.getStringList(this.clusterConfig, GobblinClusterConfigurationKeys.HELIX_INSTANCE_TAGS_KEY));
+ if (!desiredTags.isEmpty()) {
+ // The helix instance associated with this container should be consistent on helix tag
+ List<String> existedTags =
+ receiverManager.getClusterManagmentTool().getInstanceConfig(this.clusterName, this.helixInstanceName).getTags();
Review Comment:
I believe there are some yarn related assumptions (on container start registering the instance config or something) and basically we are not setting that up for all use cases. I saw a `setInstanceConfig` method in `ZKHelixAdmin` that should be called explicitly in the non-Yarn case perhaps if Helix is not automatically creating one if taskrunner joins. From testing the code, previously we never entered and tested this code block since `desiredTags` is empty for our use case. This shouldn't change the streaming logic because the `existedTags` definition is only used inside the code block so I've moved the declaration to where it's used and caught the exception that can be thrown.
--
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: dev-unsubscribe@gobblin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [gobblin] umustafi commented on a diff in pull request #3602: [GOBBLIN-1743] Ensure GobblinTaskRunner works without Yarn use
Posted by GitBox <gi...@apache.org>.
umustafi commented on code in PR #3602:
URL: https://github.com/apache/gobblin/pull/3602#discussion_r1028351561
##########
gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinTaskRunner.java:
##########
@@ -544,23 +544,25 @@ void connectHelixManagerWithRetry() {
private void addInstanceTags() {
HelixManager receiverManager = getReceiverManager();
if (receiverManager.isConnected()) {
- // The helix instance associated with this container should be consistent on helix tag
- List<String> existedTags = receiverManager.getClusterManagmentTool()
- .getInstanceConfig(this.clusterName, this.helixInstanceName).getTags();
- Set<String> desiredTags = new HashSet<>(
- ConfigUtils.getStringList(this.clusterConfig, GobblinClusterConfigurationKeys.HELIX_INSTANCE_TAGS_KEY));
- if (!desiredTags.isEmpty()) {
- // Remove tag assignments for the current Helix instance from a previous run
- for (String tag : existedTags) {
- if (!desiredTags.contains(tag))
- receiverManager.getClusterManagmentTool().removeInstanceTag(this.clusterName, this.helixInstanceName, tag);
- logger.info("Removed unrelated helix tag {} for instance {}", tag, this.helixInstanceName);
+ try {
+ Set<String> desiredTags = new HashSet<>(ConfigUtils.getStringList(this.clusterConfig, GobblinClusterConfigurationKeys.HELIX_INSTANCE_TAGS_KEY));
+ if (!desiredTags.isEmpty()) {
+ // The helix instance associated with this container should be consistent on helix tag
+ List<String> existedTags =
+ receiverManager.getClusterManagmentTool().getInstanceConfig(this.clusterName, this.helixInstanceName).getTags();
Review Comment:
It seems like Helix does not automatically register the participant as an instance config if it joins as a participant. We may need to call `setInstanceConfig` and double check our assumptions.
--
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: dev-unsubscribe@gobblin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [gobblin] Will-Lo merged pull request #3602: [GOBBLIN-1743] Ensure GobblinTaskRunner works without Yarn use
Posted by GitBox <gi...@apache.org>.
Will-Lo merged PR #3602:
URL: https://github.com/apache/gobblin/pull/3602
--
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: dev-unsubscribe@gobblin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [gobblin] umustafi commented on a diff in pull request #3602: [GOBBLIN-1743] Ensure GobblinTaskRunner works without Yarn use
Posted by GitBox <gi...@apache.org>.
umustafi commented on code in PR #3602:
URL: https://github.com/apache/gobblin/pull/3602#discussion_r1025869857
##########
gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinTaskRunner.java:
##########
@@ -544,23 +544,24 @@ void connectHelixManagerWithRetry() {
private void addInstanceTags() {
HelixManager receiverManager = getReceiverManager();
if (receiverManager.isConnected()) {
- // The helix instance associated with this container should be consistent on helix tag
- List<String> existedTags = receiverManager.getClusterManagmentTool()
- .getInstanceConfig(this.clusterName, this.helixInstanceName).getTags();
- Set<String> desiredTags = new HashSet<>(
- ConfigUtils.getStringList(this.clusterConfig, GobblinClusterConfigurationKeys.HELIX_INSTANCE_TAGS_KEY));
- if (!desiredTags.isEmpty()) {
- // Remove tag assignments for the current Helix instance from a previous run
- for (String tag : existedTags) {
- if (!desiredTags.contains(tag))
- receiverManager.getClusterManagmentTool().removeInstanceTag(this.clusterName, this.helixInstanceName, tag);
- logger.info("Removed unrelated helix tag {} for instance {}", tag, this.helixInstanceName);
+ try {
+ Set<String> desiredTags = new HashSet<>(ConfigUtils.getStringList(this.clusterConfig, GobblinClusterConfigurationKeys.HELIX_INSTANCE_TAGS_KEY));
+ if (!desiredTags.isEmpty()) {
+ // The helix instance associated with this container should be consistent on helix tag
+ List<String> existedTags =
+ receiverManager.getClusterManagmentTool().getInstanceConfig(this.clusterName, this.helixInstanceName).getTags();
+ // Remove tag assignments for the current Helix instance from a previous run
+ for (String tag : existedTags) {
+ if (!desiredTags.contains(tag))
+ receiverManager.getClusterManagmentTool().removeInstanceTag(this.clusterName, this.helixInstanceName, tag);
+ logger.info("Removed unrelated helix tag {} for instance {}", tag, this.helixInstanceName);
+ }
+ desiredTags.forEach(desiredTag -> receiverManager.getClusterManagmentTool().addInstanceTag(this.clusterName, this.helixInstanceName, desiredTag));
+ logger.info("Actual tags binding " + receiverManager.getClusterManagmentTool().getInstanceConfig(this.clusterName, this.helixInstanceName).getTags());
}
- desiredTags.forEach(desiredTag -> receiverManager.getClusterManagmentTool()
- .addInstanceTag(this.clusterName, this.helixInstanceName, desiredTag));
+ } catch (HelixException e) {
+ logger.warn("some error with helix get instance config that we're ignoring and moving on: {}", e);
Review Comment:
ah oops this was a debugging log statement. rewording
--
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: dev-unsubscribe@gobblin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [gobblin] homatthew commented on a diff in pull request #3602: [GOBBLIN-1743] Ensure GobblinTaskRunner works without Yarn use
Posted by GitBox <gi...@apache.org>.
homatthew commented on code in PR #3602:
URL: https://github.com/apache/gobblin/pull/3602#discussion_r1026986413
##########
gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinTaskRunner.java:
##########
@@ -544,23 +544,27 @@ void connectHelixManagerWithRetry() {
private void addInstanceTags() {
HelixManager receiverManager = getReceiverManager();
if (receiverManager.isConnected()) {
- // The helix instance associated with this container should be consistent on helix tag
- List<String> existedTags = receiverManager.getClusterManagmentTool()
- .getInstanceConfig(this.clusterName, this.helixInstanceName).getTags();
- Set<String> desiredTags = new HashSet<>(
- ConfigUtils.getStringList(this.clusterConfig, GobblinClusterConfigurationKeys.HELIX_INSTANCE_TAGS_KEY));
- if (!desiredTags.isEmpty()) {
- // Remove tag assignments for the current Helix instance from a previous run
- for (String tag : existedTags) {
- if (!desiredTags.contains(tag))
- receiverManager.getClusterManagmentTool().removeInstanceTag(this.clusterName, this.helixInstanceName, tag);
- logger.info("Removed unrelated helix tag {} for instance {}", tag, this.helixInstanceName);
+ try {
+ Set<String> desiredTags = new HashSet<>(
+ ConfigUtils.getStringList(this.clusterConfig, GobblinClusterConfigurationKeys.HELIX_INSTANCE_TAGS_KEY));
+ if (!desiredTags.isEmpty()) {
+ // The helix instance associated with this container should be consistent on helix tag
+ List<String> existedTags = receiverManager.getClusterManagmentTool()
+ .getInstanceConfig(this.clusterName, this.helixInstanceName).getTags();
+ // Remove tag assignments for the current Helix instance from a previous run
+ for (String tag : existedTags) {
+ if (!desiredTags.contains(tag)) {
+ receiverManager.getClusterManagmentTool().removeInstanceTag(this.clusterName, this.helixInstanceName, tag);
+ logger.info("Removed unrelated helix tag {} for instance {}", tag, this.helixInstanceName);
+ }
+ }
+ desiredTags.forEach(desiredTag -> receiverManager.getClusterManagmentTool().addInstanceTag(this.clusterName, this.helixInstanceName, desiredTag));
Review Comment:
Forgot to change this whitepsace
--
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: dev-unsubscribe@gobblin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [gobblin] codecov-commenter commented on pull request #3602: [GOBBLIN-1743] Ensure GobblinTaskRunner works without Yarn use
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #3602:
URL: https://github.com/apache/gobblin/pull/3602#issuecomment-1319383528
# [Codecov](https://codecov.io/gh/apache/gobblin/pull/3602?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#3602](https://codecov.io/gh/apache/gobblin/pull/3602?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (0888d7f) into [master](https://codecov.io/gh/apache/gobblin/commit/4e7317cb1c17181bc4f6ec2c46fae27eca8203d0?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (4e7317c) will **decrease** coverage by `3.04%`.
> The diff coverage is `14.28%`.
```diff
@@ Coverage Diff @@
## master #3602 +/- ##
============================================
- Coverage 46.90% 43.86% -3.05%
+ Complexity 10672 2063 -8609
============================================
Files 2120 408 -1712
Lines 83101 17624 -65477
Branches 9255 2154 -7101
============================================
- Hits 38979 7730 -31249
+ Misses 40549 9037 -31512
+ Partials 3573 857 -2716
```
| [Impacted Files](https://codecov.io/gh/apache/gobblin/pull/3602?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [.../org/apache/gobblin/cluster/GobblinTaskRunner.java](https://codecov.io/gh/apache/gobblin/pull/3602/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1jbHVzdGVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL2NsdXN0ZXIvR29iYmxpblRhc2tSdW5uZXIuamF2YQ==) | `62.50% <14.28%> (-1.14%)` | :arrow_down: |
| [.../gobblin/cluster/GobblinHelixTaskStateTracker.java](https://codecov.io/gh/apache/gobblin/pull/3602/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1jbHVzdGVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL2NsdXN0ZXIvR29iYmxpbkhlbGl4VGFza1N0YXRlVHJhY2tlci5qYXZh) | `62.50% <0.00%> (-6.25%)` | :arrow_down: |
| [.../java/org/apache/gobblin/cluster/SleepingTask.java](https://codecov.io/gh/apache/gobblin/pull/3602/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1jbHVzdGVyL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL2NsdXN0ZXIvU2xlZXBpbmdUYXNrLmphdmE=) | `39.39% <0.00%> (-6.07%)` | :arrow_down: |
| [...main/java/org/apache/gobblin/util/HadoopUtils.java](https://codecov.io/gh/apache/gobblin/pull/3602/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi11dGlsaXR5L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3V0aWwvSGFkb29wVXRpbHMuamF2YQ==) | `30.29% <0.00%> (-0.66%)` | :arrow_down: |
| [...time/instance/StandardGobblinInstanceLauncher.java](https://codecov.io/gh/apache/gobblin/pull/3602/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1ydW50aW1lL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3J1bnRpbWUvaW5zdGFuY2UvU3RhbmRhcmRHb2JibGluSW5zdGFuY2VMYXVuY2hlci5qYXZh) | | |
| [...rce/extractor/extract/kafka/KafkaSimpleSource.java](https://codecov.io/gh/apache/gobblin/pull/3602/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1tb2R1bGVzL2dvYmJsaW4ta2Fma2EtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3NvdXJjZS9leHRyYWN0b3IvZXh0cmFjdC9rYWZrYS9LYWZrYVNpbXBsZVNvdXJjZS5qYXZh) | | |
| [...\_catalog/PackagedTemplatesJobCatalogDecorator.java](https://codecov.io/gh/apache/gobblin/pull/3602/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1ydW50aW1lL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3J1bnRpbWUvam9iX2NhdGFsb2cvUGFja2FnZWRUZW1wbGF0ZXNKb2JDYXRhbG9nRGVjb3JhdG9yLmphdmE=) | | |
| [.../service/modules/orchestration/AzkabanSuccess.java](https://codecov.io/gh/apache/gobblin/pull/3602/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1tb2R1bGVzL2dvYmJsaW4tYXprYWJhbi9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvZ29iYmxpbi9zZXJ2aWNlL21vZHVsZXMvb3JjaGVzdHJhdGlvbi9BemthYmFuU3VjY2Vzcy5qYXZh) | | |
| [...ava/org/apache/gobblin/runtime/util/GsonUtils.java](https://codecov.io/gh/apache/gobblin/pull/3602/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1ydW50aW1lL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3J1bnRpbWUvdXRpbC9Hc29uVXRpbHMuamF2YQ==) | | |
| [...org/apache/gobblin/source/jdbc/MysqlExtractor.java](https://codecov.io/gh/apache/gobblin/pull/3602/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-Z29iYmxpbi1tb2R1bGVzL2dvYmJsaW4tc3FsL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9nb2JibGluL3NvdXJjZS9qZGJjL015c3FsRXh0cmFjdG9yLmphdmE=) | | |
| ... and [1709 more](https://codecov.io/gh/apache/gobblin/pull/3602/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
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: dev-unsubscribe@gobblin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [gobblin] Will-Lo commented on a diff in pull request #3602: [GOBBLIN-1743] Ensure GobblinTaskRunner works without Yarn use
Posted by GitBox <gi...@apache.org>.
Will-Lo commented on code in PR #3602:
URL: https://github.com/apache/gobblin/pull/3602#discussion_r1025865028
##########
gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinTaskRunner.java:
##########
@@ -544,23 +544,24 @@ void connectHelixManagerWithRetry() {
private void addInstanceTags() {
HelixManager receiverManager = getReceiverManager();
if (receiverManager.isConnected()) {
- // The helix instance associated with this container should be consistent on helix tag
- List<String> existedTags = receiverManager.getClusterManagmentTool()
- .getInstanceConfig(this.clusterName, this.helixInstanceName).getTags();
- Set<String> desiredTags = new HashSet<>(
- ConfigUtils.getStringList(this.clusterConfig, GobblinClusterConfigurationKeys.HELIX_INSTANCE_TAGS_KEY));
- if (!desiredTags.isEmpty()) {
- // Remove tag assignments for the current Helix instance from a previous run
- for (String tag : existedTags) {
- if (!desiredTags.contains(tag))
- receiverManager.getClusterManagmentTool().removeInstanceTag(this.clusterName, this.helixInstanceName, tag);
- logger.info("Removed unrelated helix tag {} for instance {}", tag, this.helixInstanceName);
+ try {
+ Set<String> desiredTags = new HashSet<>(ConfigUtils.getStringList(this.clusterConfig, GobblinClusterConfigurationKeys.HELIX_INSTANCE_TAGS_KEY));
+ if (!desiredTags.isEmpty()) {
+ // The helix instance associated with this container should be consistent on helix tag
+ List<String> existedTags =
+ receiverManager.getClusterManagmentTool().getInstanceConfig(this.clusterName, this.helixInstanceName).getTags();
+ // Remove tag assignments for the current Helix instance from a previous run
+ for (String tag : existedTags) {
+ if (!desiredTags.contains(tag))
+ receiverManager.getClusterManagmentTool().removeInstanceTag(this.clusterName, this.helixInstanceName, tag);
+ logger.info("Removed unrelated helix tag {} for instance {}", tag, this.helixInstanceName);
+ }
+ desiredTags.forEach(desiredTag -> receiverManager.getClusterManagmentTool().addInstanceTag(this.clusterName, this.helixInstanceName, desiredTag));
+ logger.info("Actual tags binding " + receiverManager.getClusterManagmentTool().getInstanceConfig(this.clusterName, this.helixInstanceName).getTags());
}
- desiredTags.forEach(desiredTag -> receiverManager.getClusterManagmentTool()
- .addInstanceTag(this.clusterName, this.helixInstanceName, desiredTag));
+ } catch (HelixException e) {
+ logger.warn("some error with helix get instance config that we're ignoring and moving on: {}", e);
Review Comment:
Can we reword this error to provide more context on when it's safe and unsafe? i.e.
```
"Error with Helix getting instance config tags used in YARN cluster configuration. Will ignore and attempt to move on"
```
--
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: dev-unsubscribe@gobblin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [gobblin] homatthew commented on a diff in pull request #3602: [GOBBLIN-1743] Ensure GobblinTaskRunner works without Yarn use
Posted by GitBox <gi...@apache.org>.
homatthew commented on code in PR #3602:
URL: https://github.com/apache/gobblin/pull/3602#discussion_r1026996560
##########
gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinTaskRunner.java:
##########
@@ -544,23 +544,25 @@ void connectHelixManagerWithRetry() {
private void addInstanceTags() {
HelixManager receiverManager = getReceiverManager();
if (receiverManager.isConnected()) {
- // The helix instance associated with this container should be consistent on helix tag
- List<String> existedTags = receiverManager.getClusterManagmentTool()
- .getInstanceConfig(this.clusterName, this.helixInstanceName).getTags();
- Set<String> desiredTags = new HashSet<>(
- ConfigUtils.getStringList(this.clusterConfig, GobblinClusterConfigurationKeys.HELIX_INSTANCE_TAGS_KEY));
- if (!desiredTags.isEmpty()) {
- // Remove tag assignments for the current Helix instance from a previous run
- for (String tag : existedTags) {
- if (!desiredTags.contains(tag))
- receiverManager.getClusterManagmentTool().removeInstanceTag(this.clusterName, this.helixInstanceName, tag);
- logger.info("Removed unrelated helix tag {} for instance {}", tag, this.helixInstanceName);
+ try {
+ Set<String> desiredTags = new HashSet<>(ConfigUtils.getStringList(this.clusterConfig, GobblinClusterConfigurationKeys.HELIX_INSTANCE_TAGS_KEY));
+ if (!desiredTags.isEmpty()) {
+ // The helix instance associated with this container should be consistent on helix tag
+ List<String> existedTags =
+ receiverManager.getClusterManagmentTool().getInstanceConfig(this.clusterName, this.helixInstanceName).getTags();
Review Comment:
Correct. The current implementation doesn't affect streaming logic and should work. I am pointing out that a missing instance config is an indication of a larger problem. i.e. I don't think this change addresses the core issue which is that we are missing an instance config which causes the exception.
```
instance [instance] does not exist in cluster [cluster]
```
##########
gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinTaskRunner.java:
##########
@@ -544,23 +544,25 @@ void connectHelixManagerWithRetry() {
private void addInstanceTags() {
HelixManager receiverManager = getReceiverManager();
if (receiverManager.isConnected()) {
- // The helix instance associated with this container should be consistent on helix tag
- List<String> existedTags = receiverManager.getClusterManagmentTool()
- .getInstanceConfig(this.clusterName, this.helixInstanceName).getTags();
- Set<String> desiredTags = new HashSet<>(
- ConfigUtils.getStringList(this.clusterConfig, GobblinClusterConfigurationKeys.HELIX_INSTANCE_TAGS_KEY));
- if (!desiredTags.isEmpty()) {
- // Remove tag assignments for the current Helix instance from a previous run
- for (String tag : existedTags) {
- if (!desiredTags.contains(tag))
- receiverManager.getClusterManagmentTool().removeInstanceTag(this.clusterName, this.helixInstanceName, tag);
- logger.info("Removed unrelated helix tag {} for instance {}", tag, this.helixInstanceName);
+ try {
+ Set<String> desiredTags = new HashSet<>(ConfigUtils.getStringList(this.clusterConfig, GobblinClusterConfigurationKeys.HELIX_INSTANCE_TAGS_KEY));
+ if (!desiredTags.isEmpty()) {
Review Comment:
Does non standalone gobblin mean yarn? Does that mean all usages of yarn don't use the GobblinYarnTaskRunner?
##########
gobblin-cluster/src/main/java/org/apache/gobblin/cluster/GobblinTaskRunner.java:
##########
@@ -544,23 +544,27 @@ void connectHelixManagerWithRetry() {
private void addInstanceTags() {
HelixManager receiverManager = getReceiverManager();
if (receiverManager.isConnected()) {
- // The helix instance associated with this container should be consistent on helix tag
- List<String> existedTags = receiverManager.getClusterManagmentTool()
- .getInstanceConfig(this.clusterName, this.helixInstanceName).getTags();
- Set<String> desiredTags = new HashSet<>(
- ConfigUtils.getStringList(this.clusterConfig, GobblinClusterConfigurationKeys.HELIX_INSTANCE_TAGS_KEY));
- if (!desiredTags.isEmpty()) {
- // Remove tag assignments for the current Helix instance from a previous run
- for (String tag : existedTags) {
- if (!desiredTags.contains(tag))
- receiverManager.getClusterManagmentTool().removeInstanceTag(this.clusterName, this.helixInstanceName, tag);
- logger.info("Removed unrelated helix tag {} for instance {}", tag, this.helixInstanceName);
+ try {
+ Set<String> desiredTags = new HashSet<>(
+ ConfigUtils.getStringList(this.clusterConfig, GobblinClusterConfigurationKeys.HELIX_INSTANCE_TAGS_KEY));
+ if (!desiredTags.isEmpty()) {
+ // The helix instance associated with this container should be consistent on helix tag
+ List<String> existedTags = receiverManager.getClusterManagmentTool()
+ .getInstanceConfig(this.clusterName, this.helixInstanceName).getTags();
+ // Remove tag assignments for the current Helix instance from a previous run
+ for (String tag : existedTags) {
+ if (!desiredTags.contains(tag)) {
+ receiverManager.getClusterManagmentTool().removeInstanceTag(this.clusterName, this.helixInstanceName, tag);
+ logger.info("Removed unrelated helix tag {} for instance {}", tag, this.helixInstanceName);
+ }
+ }
+ desiredTags.forEach(desiredTag -> receiverManager.getClusterManagmentTool().addInstanceTag(this.clusterName, this.helixInstanceName, desiredTag));
Review Comment:
Forgot to change this white space back
--
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: dev-unsubscribe@gobblin.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org