Skip to content

Make Uploader pluggable through secor.properties#207

Merged
HenryCaiHaiying merged 1 commit intopinterest:masterfrom
lucamilanesio:pluggable-uploader-policy
May 4, 2016
Merged

Make Uploader pluggable through secor.properties#207
HenryCaiHaiying merged 1 commit intopinterest:masterfrom
lucamilanesio:pluggable-uploader-policy

Conversation

@lucamilanesio
Copy link
Copy Markdown
Contributor

Allow custom logic to be applied to flush locally stored topic files
to the underlying storage system.

Currently, only a single logic can be applied, and it is coded in the Uploader class.
By making the class pluggable and configurable through secor.upload.class property
we can just override the applyPolicy() and customize the logic on how to flush data.

This change is fully backward compatible. When property is not configured the Uploader
class is instantiated as before.

@lucamilanesio lucamilanesio force-pushed the pluggable-uploader-policy branch 2 times, most recently from dac99a8 to 5af43c8 Compare April 28, 2016 15:38
package com.pinterest.secor.common;

import com.google.api.client.repackaged.com.google.common.base.Strings;
import com.pinterest.secor.uploader.Uploader;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Doesn't quite like this added dependency to secor.uploader package, I think you can have this property have a default value in secor.common.properties (there are quite a few config property specifying classname in secor.common.properties already)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@HenryCaiHaiying thanks for the feedback, makes sense, will include in my next amended commit.

@lucamilanesio lucamilanesio force-pushed the pluggable-uploader-policy branch from 5af43c8 to 16f95b9 Compare May 3, 2016 10:19
@lucamilanesio
Copy link
Copy Markdown
Contributor Author

@HenryCaiHaiying all changes you requested have been applied, could you have a second look please? Thank you in advance. Luca.

UploadManager uploadManager) {
this(config, offsetTracker, fileRegistry, uploadManager,
new ZookeeperConnector(config));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you add the javadoc to this 'init' method?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Will do, even though there was no JavaDoc on the previous constructor ... but I can deduct what is what from the code :-)

@lucamilanesio lucamilanesio force-pushed the pluggable-uploader-policy branch from 16f95b9 to 7c1daa1 Compare May 4, 2016 20:08
Allow custom logic to be applied to flush locally stored topic files
to the underlying storage system.
@lucamilanesio lucamilanesio force-pushed the pluggable-uploader-policy branch from 7c1daa1 to ca0cf23 Compare May 4, 2016 20:08
@lucamilanesio
Copy link
Copy Markdown
Contributor Author

@HenryCaiHaiying thanks for the extra reviews, amended the commit and rebased on current master.

@lucamilanesio
Copy link
Copy Markdown
Contributor Author

Build is green, hope we should be good to go 😄

@HenryCaiHaiying HenryCaiHaiying merged commit 57317a5 into pinterest:master May 4, 2016
@HenryCaiHaiying
Copy link
Copy Markdown
Contributor

Looks good, thanks for the contribution.

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.

2 participants