Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[5889] Bmoric/add application name [2/3] #7267

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions airbyte-workers/src/main/java/io/airbyte/workers/Application.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
/*
* Copyright (c) 2021 Airbyte, Inc., all rights reserved.
*/

package io.airbyte.workers;

public interface Application {

default String getApplicationName() {
// This value should only be used in the U-Test, it is an empty string instead of airbyte-test in
// order to avoid displaying airbyte-test in prod
return "";
}
Comment on lines +9 to +13
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By convention, we do not usually use a default in an interface if the default implementation is for testing only. The reason for this is that with this default, it is easy for a developer to make a mistake using this interface because the compiler won't warn them that they need to override method. If they fail to do that, then test code will accidentally run in production. Instead we would usually structure it something like this, so that the compiler complains if the developer makes a mistake.

Suggested change
default String getApplicationName() {
// This value should only be used in the U-Test, it is an empty string instead of airbyte-test in
// order to avoid displaying airbyte-test in prod
return "";
}
String getApplicationName();
class TestApplication implements Application {
@Override
public String getApplicationName() {
// This value should only be used in the U-Test, it is an empty string instead of airbyte-test in
// order to avoid displaying airbyte-test in prod
return "";
}
}


}
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ public void start() throws Exception {
* transform-config scripts (to translate Airbyte Catalogs into Dbt profiles file). Thus, we depend
* on the NormalizationRunner to configure the dbt project with the appropriate destination settings
* and pull the custom git repository into the workspace.
*
* <p>
* Once the workspace folder/files is setup to run, we invoke the custom transformation command as
* provided by the user to execute whatever extra transformation has been implemented.
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

public class DbtTransformationWorker implements Worker<OperatorDbtInput, Void> {
public class DbtTransformationWorker implements Worker<OperatorDbtInput, Void>, Application {

private static final Logger LOGGER = LoggerFactory.getLogger(DbtTransformationWorker.class);

Expand Down Expand Up @@ -77,4 +77,9 @@ public void cancel() {
}
}

@Override
public String getApplicationName() {
return "normalization-worker";
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -86,4 +86,9 @@ public void cancel() {
WorkerUtils.cancelProcess(process);
}

@Override
public String getApplicationName() {
return "check-connection-worker";
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -81,4 +81,9 @@ public void cancel() {
WorkerUtils.cancelProcess(process);
}

@Override
public String getApplicationName() {
return "discover-worker";
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -81,4 +81,9 @@ public void cancel() {
WorkerUtils.cancelProcess(process);
}

@Override
public String getApplicationName() {
return "get-spec-worker";
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -82,4 +82,9 @@ public void cancel() {
}
}

@Override
public String getApplicationName() {
return "normalization-worker";
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -271,4 +271,9 @@ public void cancel() {

}

@Override
public String getApplicationName() {
return "sync-worker";
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -25,4 +25,9 @@ public void cancel() {
// no-op
}

@Override
public String getApplicationName() {
return "airbyte-echo-worker";
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@

import java.nio.file.Path;

public interface Worker<InputType, OutputType> {
public interface Worker<InputType, OutputType> extends Application {

/**
* Blocking call to run the worker's workflow. Once this is complete, getStatus should return either
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,11 @@
package io.airbyte.workers.protocols;

import io.airbyte.config.State;
import io.airbyte.workers.Application;
import java.util.Optional;
import java.util.function.Consumer;

public interface MessageTracker<T> extends Consumer<T> {
public interface MessageTracker<T> extends Consumer<T>, Application {
Copy link
Contributor

@michel-tricot michel-tricot Oct 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this doesn't seem right. Message Tracker isn't an Application. I wonder if the way to track name is wrong or just the naming of this interface. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that we need make sure that I properly understand how granular we want to log. My initial thought was that we have a different prefix within the same application (a different prefix per step). So if we want to go in this direction we will probably need to rename the interface. If not I should remove the Application interface and move this method to the worker interface.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree the iface name should change.

Will follow up offline, but I'm am not understanding the granularity we are going for right now. The issue is focused on distinguishing source, destination, and platform logs from each other. This PR is focused on distinguishing worker logs from each other, and I'm a little less clear on why that is valuable.

Copy link
Contributor Author

@benmoriceau benmoriceau Oct 22, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I sync up offline with Michel on that. I will abandon the interface and this review for something more suited to what is needed.


@Override
void accept(T message);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,4 +52,9 @@ public Optional<State> getOutputState() {
return Optional.ofNullable(outputState.get());
}

@Override
public String getApplicationName() {
return "airbyte-message-tracker";
}

}