-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Kube Logs are stored and retrieved from Cloud Storage. #4053
Conversation
… preserving local logs.
… substitution in configuration.
…e aws s3 v2 sdk. Also rename aws credentials env var to match what is expected by aws.
<PatternLayout pattern="${default-worker-file-pattern}"/> | ||
</File> | ||
</Route> | ||
</Routes> | ||
<IdlePurgePolicy timeToLive="15" timeUnit="minutes"/> | ||
</Routing> | ||
<Routing name="LogSplitCloud"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will comment why we had to configure in this way - essentially only accepts one appender.
|
||
public class LogHelpers { | ||
|
||
// if you update these values, you must also update log4j2.xml | ||
private static final Logger LOGGER = LoggerFactory.getLogger(LogHelpers.class); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should all the logging related stuff be moved to it's own module? or can it stay in models for now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can stay for now but should be split eventually.
airbyte-server/src/main/java/io/airbyte/server/converters/JobConverter.java
Show resolved
Hide resolved
This reverts commit 19db827.
…g this ID. Consolidate all MDC operations to setMDC function in WorkerUtils. Remove unused functions.
…orking. Move logging test to be part of Kube integration tests.
@@ -50,7 +50,7 @@ | |||
private static final ObjectWriter OBJECT_WRITER = OBJECT_MAPPER.writer(new JsonPrettyPrinter()); | |||
|
|||
private static ObjectMapper initMapper() { | |||
final ObjectMapper result = new ObjectMapper(); | |||
final ObjectMapper result = new ObjectMapper().findAndRegisterModules(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why?
@@ -3,6 +3,5 @@ plugins { | |||
} | |||
|
|||
dependencies { | |||
implementation 'org.apache.commons:commons-compress:1.20' | |||
implementation 'org.apache.commons:commons-lang3:3.11' | |||
// Dependencies for this module should be specified in the top-level build.gradle. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because of https://github.com/airbytehq/airbyte/blob/master/build.gradle#L181, if everything is depending on airbyte-commons, it seems cleaner to have all of common's dependencies in the build.gradle as well.
@@ -45,7 +45,7 @@ | |||
|
|||
public static final YAMLFactory YAML_FACTORY = new YAMLFactory(); | |||
// Object Mapper is thread-safe | |||
private static final ObjectMapper OBJECT_MAPPER = new ObjectMapper(YAML_FACTORY); | |||
private static final ObjectMapper OBJECT_MAPPER = new ObjectMapper(YAML_FACTORY).findAndRegisterModules(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why?
|
||
public class LogHelpers { | ||
|
||
// if you update these values, you must also update log4j2.xml | ||
private static final Logger LOGGER = LoggerFactory.getLogger(LogHelpers.class); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can stay for now but should be split eventually.
airbyte-config/models/src/main/java/io/airbyte/config/helpers/LogHelpers.java
Outdated
Show resolved
Hide resolved
airbyte-scheduler/app/src/main/java/io/airbyte/scheduler/app/JobSubmitter.java
Outdated
Show resolved
Hide resolved
/test connector=connectors/destination-redshift
|
/test connector=connectors/destination-snowflake
|
/test connector=connectors/destination-s3
|
Remaining work
[X] Modify API to read from Cloud. In general, we should defer to local if the log file exists, and Cloud Storage if it does not.
[x] Figure out configuration to also support GCS.
[X] Modify Kube deployment to support Cloud.
[x] Add better comments and log test to the Docker and Kube acceptance tests.
Future work (will create tickets for these):