Skip to content

Conversation

@woshigaopp
Copy link
Contributor

This pull request introduces the new AutoMQ Log Uploader module, which provides asynchronous S3 log upload functionality for Log4j 1.x applications. It refactors and relocates key log uploading classes, adds configuration constants, and implements a robust configuration and startup process for the uploader. The changes are grouped into module introduction/documentation, configuration and integration, and codebase refactoring.

Module introduction & documentation:

  • Added a comprehensive README.md for the new automq-log-uploader module, detailing its purpose, integration steps, configuration options, and extension points for custom setups.

Configuration and integration:

  • Added build.gradle for the new module, specifying dependencies and repository settings for building and integrating the uploader.
  • Introduced LogConfigConstants.java to centralize all configuration keys and defaults for S3 log uploading, improving maintainability and clarity.
  • Added DefaultS3LogConfig.java, which loads and normalizes configuration from properties, constructs the S3 object storage URI, and sets up leader election strategies for log uploading and cleanup.

Codebase refactoring and improvements:

  • Refactored and relocated LogUploader.java and LogRecorder.java from the automq-shell submodule to the new automq-log-uploader package, decoupling them from application-specific dependencies and improving error messages in validation logic. [1] [2] [3]
  • Simplified the startup and shutdown logic in LogUploader.java, replacing the singleton/bean pattern with explicit configuration and thread management, and removed unused async/future initialization code. [1] [2]

@@ -0,0 +1,385 @@
package com.automq.log.uploader.selector.kafka;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add Apache License header

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -0,0 +1,19 @@
plugins {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why does automq-log-uploader have independent build.gradle instead of keeping the same pattern as others modules.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's like automq-shell.

/**
* Leader election based on Kafka consumer group membership.
*/
public class KafkaLogLeaderSelectorProvider implements LogUploaderNodeSelectorProvider {
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's the KafkaLogLeaderSelectorProvider used for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

now use activeControloler instead

int nodeId,
Map<String, String> config) {
LogUploaderNodeSelectorType type = LogUploaderNodeSelectorType.fromString(typeString);
switch (type) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

What are the actual application scenarios in AutoMQ of these different types?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

now use activeControloler instead

this(null);
}

public DefaultS3LogConfig(Properties overrideProps) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we could directly use bucketURI as S3Stream instead of defining separated configs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it is same with old, DefaultS3LogConfig is not just about S3 configuration - it's a comprehensive log uploader configuration abstraction that encompasses multiple concerns beyond simple S3 connectivity.

import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

public class S3RollingFileAppender extends RollingFileAppender {
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's a little bit complex. Maybe we could keep the almost same S3RollingFileAppender and LogUploader as before.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

just provider custom log class, not so complex

log4j.appender.requestAppender.layout.ConversionPattern=[%d] %p %m (%c)%n

log4j.appender.cleanerAppender=com.automq.shell.log.S3RollingFileAppender
log4j.appender.cleanerAppender=com.automq.log.uploader.S3RollingFileAppender
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's a little bit strange to have S3RollingFileAppender in a uploader package. Maybe it should in com.automq.log package

}
LOGGER.info("Log upload a thread is running.");
try {
String objectKey = getObjectKey();
Copy link
Collaborator

Choose a reason for hiding this comment

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

It seams the object path is same pattern for connector and broker?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, I don't see any problem with same pattern

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants