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

feat: new getLog task #1059

Merged
merged 6 commits into from
Mar 16, 2023
Merged

feat: new getLog task #1059

merged 6 commits into from
Mar 16, 2023

Conversation

Skraye
Copy link
Member

@Skraye Skraye commented Mar 13, 2023

No description provided.

Copy link
Member

@loicmathieu loicmathieu left a comment

Choose a reason for hiding this comment

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

Maybe add a metric with the number of lines.

@Skraye Skraye force-pushed the feat/get-log-task branch 2 times, most recently from c3e7db2 to 838f608 Compare March 13, 2023 14:04
)
}
)
public class Fetch extends Task implements RunnableTask<Fetch.Output> {
Copy link
Member

Choose a reason for hiding this comment

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

You need to support the executionId like in Email or Slack Notification.
People use trigger flows for notification most of the time, and they want to get the log from an previous execution.

public Output run(RunContext runContext) throws Exception {
String executionId = (String) new HashMap<>((Map<String, Object>) runContext.getVariables().get("execution")).get("id");
LogRepositoryInterface logRepository = runContext.getApplicationContext().getBean(LogRepositoryInterface.class);
List<LogEntry> logs = new ArrayList<>();
Copy link
Member

Choose a reason for hiding this comment

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

You are mounting the full log in memory with this, you need to flush the data directly on the filesystems

LogRepositoryInterface logRepository = runContext.getApplicationContext().getBean(LogRepositoryInterface.class);
List<LogEntry> logs = new ArrayList<>();

if(this.tasksId != null){
Copy link
Member

Choose a reason for hiding this comment

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

Best is to used the find( to avoid mounting the whole data in memory

@Skraye Skraye force-pushed the feat/get-log-task branch from 3e93a64 to 6aee866 Compare March 14, 2023 10:51
@@ -358,6 +358,7 @@ subprojects {

task sourcesJar(type: Jar) {
dependsOn = [':core:copyGradleProperties']
dependsOn = [':ui:assembleFrontend']
Copy link
Member

Choose a reason for hiding this comment

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

Is it needed or is it a leftover of something else ?
Seems not related to this PR

Copy link
Member Author

Choose a reason for hiding this comment

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

Probably leftover of this : f4e378c

public class Fetch extends Task implements RunnableTask<Fetch.Output> {

@Schema(
title = "Filter on specific execution"
Copy link
Member

Choose a reason for hiding this comment

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

Add a description explaining that if empty we will fetch the log of the current execution.


@Override
public Output run(RunContext runContext) throws Exception {
String executionId = this.executionId != null ? runContext.render(this.executionId) : (String) new HashMap<>((Map<String, Object>) runContext.getVariables().get("execution")).get("id");
Copy link
Member

Choose a reason for hiding this comment

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

Why do you create a new HashMap ? You should be able to get the execution ID directly from the map returned but the get("execution")

@Getter
public static class Output implements io.kestra.core.models.tasks.Output {
@Schema(
title = "The size of the rows fetch"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
title = "The size of the rows fetch"
title = "The size of the fetched rows"

private Long size;

@Schema(
title = "The uri of store result",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
title = "The uri of store result",
title = "The uri of stored results",

@Skraye Skraye force-pushed the feat/get-log-task branch from 6aee866 to fab714e Compare March 15, 2023 11:02
@Skraye Skraye force-pushed the feat/get-log-task branch from fab714e to 0e101c7 Compare March 15, 2023 11:04
@tchiotludo tchiotludo merged commit 126c39d into develop Mar 16, 2023
@tchiotludo tchiotludo deleted the feat/get-log-task branch March 16, 2023 16:59
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