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
SAMZA-1788: Add LocationIdProvider abstraction. #585
Conversation
@xinyuiscool @sborya |
|
||
import org.apache.samza.util.Util; | ||
|
||
public class DefaultLocationIdProviderFactory implements LocationIdProviderFactory { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding java-docs here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
*/ | ||
public interface LocationIdProvider { | ||
|
||
LocationId getLocationId(Config config); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prefer re-thinking these interfaces as follows:
public interface LocationIdProvider {
LocationId getLocationId();
}
and the corresponding factory interface as:
public interface LocationIdProviderFactory {
LocationIdProvider getLocationIdProvider(Config config);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the review.
*/ | ||
public interface LocationIdProvider { | ||
|
||
LocationId getLocationId(Config config); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
|
||
import org.apache.samza.util.Util; | ||
|
||
public class DefaultLocationIdProviderFactory implements LocationIdProviderFactory { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
Looks like there are test failures. can you take a look? Otherwise, LGTM |
Currently in standalone, by default hostName of the standalone processor is used as LocationId. However, for containerized environments like azure cloud, kubernetes this defaulting does not work. Standalone processors can be launched from different kubernetes container on a physical machine(where each kubernetes container has different locatliyID than other kubernetes container within same machine).
To solve this problem, we introduce locationID abstraction to allow users to plugin a uniqueId identifying the execution environment of the processor.
In containerized environments, LocationId is a composite key of multiple fields: (sliceId, containerId, hostname) By default hostname will be used as LocationId(if not configured by the user).
All the processors of an application registered from an locationID should be able to share(read/write) their local state stores. Any custom LocationIdProvider is expected to honor this contract when generating the locationID.
This patch is part of SEP-11. Please refer to it for more details.