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 Google Storage #9519

Merged
merged 2 commits into from
Mar 19, 2020

Conversation

zachjsh
Copy link
Contributor

@zachjsh zachjsh commented Mar 15, 2020

Description

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

  • implement ability to delete all segments from Google 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 Google storage

* implement ability to delete all segments from Google 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.

lgtm (sorry for all the formatting nitpicking)

public GoogleDataSegmentKiller(
final GoogleStorage storage,
GoogleAccountConfig accountConfig,
GoogleInputDataConfig inputDataConfig)
Copy link
Member

Choose a reason for hiding this comment

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

formatting

Suggested change
GoogleInputDataConfig inputDataConfig)
GoogleInputDataConfig inputDataConfig
)

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 106 to 107
LOG.info("Deleting all segment files from gs location [bucket: '%s' prefix: '%s']",
accountConfig.getBucket(), accountConfig.getPrefix()
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
LOG.info("Deleting all segment files from gs location [bucket: '%s' prefix: '%s']",
accountConfig.getBucket(), accountConfig.getPrefix()
LOG.info(
"Deleting all segment files from gs location [bucket: '%s' prefix: '%s']",
accountConfig.getBucket(),
accountConfig.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

GoogleTaskLogsConfig config,
GoogleStorage storage,
GoogleInputDataConfig inputDataConfig,
CurrentTimeMillisSupplier timeSupplier)
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
CurrentTimeMillisSupplier timeSupplier)
CurrentTimeMillisSupplier timeSupplier
)

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 174 to 176
LOG.info("Deleting all task logs from gs location [bucket: '%s' prefix: '%s'].",
config.getBucket(), 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.

Suggested change
LOG.info("Deleting all task logs from gs location [bucket: '%s' prefix: '%s'].",
config.getBucket(), config.getPrefix()
);
LOG.info(
"Deleting all task logs from gs location [bucket: '%s' prefix: '%s'].",
config.getBucket(),
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 185 to 187
LOG.info("Deleting all task logs from gs location [bucket: '%s' prefix: '%s'] older than %s.",
config.getBucket(), 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.

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

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 168 to 170
log.info("Deleting all task logs from s3 location [bucket: '%s' prefix: '%s'].",
config.getS3Bucket(), config.getS3Prefix()
);
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
log.info("Deleting all task logs from s3 location [bucket: '%s' prefix: '%s'].",
config.getS3Bucket(), config.getS3Prefix()
);
log.info(
"Deleting all task logs from s3 location [bucket: '%s' prefix: '%s'].",
config.getS3Bucket(),
config.getS3Prefix()
);

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

@clintropolis clintropolis merged commit 8387354 into apache:master Mar 19, 2020
@zachjsh zachjsh deleted the IMPLY-2390 branch March 19, 2020 06:59
@jihoonson jihoonson added this to the 0.18.0 milestone Mar 26, 2020
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