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

Ability to Delete task logs and segments from Azure Storage #9523

Merged
merged 2 commits into from
Mar 19, 2020

Conversation

zachjsh
Copy link
Contributor

@zachjsh zachjsh commented Mar 17, 2020

Description

  • implement ability to delete all tasks logs or all task logs
    written before a particular date when written to Azure storage

  • implement ability to delete all segments from Azure deep storage

This PR has:

  • been self-reviewed.
  • added documentation for new or modified features or behaviors.
  • added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
  • added or updated version, license, or notice information in licenses.yaml
  • added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
  • added unit tests or modified existing tests to cover new code paths.
  • added integration tests.
  • been tested in a test Druid cluster.

* implement ability to delete all tasks logs or all task logs
  written before a particular date when written to Azure storage

* implement ability to delete all segments from Azure deep storage
Copy link
Member

@clintropolis clintropolis left a comment

Choose a reason for hiding this comment

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

overall lgtm 🤘, it seems like the code coverage is failing but only for java 11 for some reason?

Comment on lines 91 to 93
log.info("Deleting all segment files from Azure storage location [bucket: '%s' prefix: '%s']",
segmentConfig.getContainer(), segmentConfig.getPrefix()
);
Copy link
Member

Choose a reason for hiding this comment

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

nit: formatting

Suggested change
log.info("Deleting all segment files from Azure storage location [bucket: '%s' prefix: '%s']",
segmentConfig.getContainer(), segmentConfig.getPrefix()
);
log.info(
"Deleting all segment files from Azure storage location [bucket: '%s' prefix: '%s']",
segmentConfig.getContainer(),
segmentConfig.getPrefix()
);

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

);
}
catch (Exception e) {
log.error("Error occurred while deleting segment files from s3. Error: %s", e.getMessage());
Copy link
Member

Choose a reason for hiding this comment

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

Heh, should probably be

Suggested change
log.error("Error occurred while deleting segment files from s3. Error: %s", e.getMessage());
log.error("Error occurred while deleting segment files from Azure. Error: %s", e.getMessage());

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

);
}
catch (Exception e) {
log.error("Error occurred while deleting task log files from s3. Error: %s", e.getMessage());
Copy link
Member

Choose a reason for hiding this comment

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

same wrong cloud

Suggested change
log.error("Error occurred while deleting task log files from s3. Error: %s", e.getMessage());
log.error("Error occurred while deleting task log files from Azure. Error: %s", e.getMessage());

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

Comment on lines 172 to 174
log.info("Deleting all task logs from Azure storage location [bucket: %s prefix: %s].",
config.getContainer(), config.getPrefix()
);
Copy link
Member

Choose a reason for hiding this comment

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

same comment about 1 line per arg formatting if spans multiple lines

Suggested change
log.info("Deleting all task logs from Azure storage location [bucket: %s prefix: %s].",
config.getContainer(), config.getPrefix()
);
log.info(
"Deleting all task logs from Azure storage location [bucket: %s prefix: %s].",
config.getContainer(),
config.getPrefix()
);

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

Comment on lines 183 to 185
log.info("Deleting all task logs from Azure storage location [bucket: '%s' prefix: '%s'] older than %s.",
config.getContainer(), config.getPrefix(), new Date(timestamp)
);
Copy link
Member

Choose a reason for hiding this comment

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

same formatting nit:

Suggested change
log.info("Deleting all task logs from Azure storage location [bucket: '%s' prefix: '%s'] older than %s.",
config.getContainer(), config.getPrefix(), new Date(timestamp)
);
log.info(
"Deleting all task logs from Azure storage location [bucket: '%s' prefix: '%s'] older than %s.",
config.getContainer(),
config.getPrefix(),
new Date(timestamp)
);

sorry there isn't a style rule for this, last we checked it wasn't possible to do, but it's been a while so it might be worth checking on again...

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

Copy link
Member

@clintropolis clintropolis left a comment

Choose a reason for hiding this comment

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

👍

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

Successfully merging this pull request may close these issues.

4 participants