-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
[BEAM-1127] Create an unique source when using a JMS topic to avoid p… #1573
Conversation
R: @dhalperi |
Refer to this link for build results (access rights to CI server needed): |
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.
A few comments, possibly uninformed.
@@ -236,8 +236,15 @@ public UnboundedJmsSource( | |||
public List<UnboundedJmsSource> generateInitialSplits( | |||
int desiredNumSplits, PipelineOptions options) throws Exception { | |||
List<UnboundedJmsSource> sources = new ArrayList<>(); | |||
for (int i = 0; i < desiredNumSplits; i++) { | |||
if (topic != null) { | |||
// in the case of a topic, we create a single source, so an unique subscriber, to avoid |
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.
I might be totally uninformed, but is it possible to make a queue out of a topic? Said differently, is it possible to separate a given topic into multiple queues?
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.
It's possible but on the broker side. In ActiveMQ, it's what a Virtual Topic.
@@ -236,8 +236,15 @@ public UnboundedJmsSource( | |||
public List<UnboundedJmsSource> generateInitialSplits( | |||
int desiredNumSplits, PipelineOptions options) throws Exception { | |||
List<UnboundedJmsSource> sources = new ArrayList<>(); | |||
for (int i = 0; i < desiredNumSplits; i++) { | |||
if (topic != null) { |
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.
Is it reasonable to add a unit test that fails before this change and passes after this change, just to make sure we don't regress in the future?
Concur that this behavior should be tested. LGTM otherwise. |
Gonna rebase and add a test to check the number of source created is correct depending of the destination (topic or queue). Thanks ! |
Refer to this link for build results (access rights to CI server needed): Build result: FAILURE[...truncated 11557 lines...] at org.eclipse.aether.internal.impl.DefaultRepositorySystem.resolveDependencies(DefaultRepositorySystem.java:384) at org.apache.maven.project.DefaultProjectDependenciesResolver.resolve(DefaultProjectDependenciesResolver.java:205) ... 35 moreCaused by: org.eclipse.aether.resolution.ArtifactResolutionException: The following artifacts could not be resolved: org.apache.apex:apex-common:jar:3.5.0-SNAPSHOT, org.apache.apex:apex-engine:jar:3.5.0-SNAPSHOT: Could not find artifact org.apache.apex:apex-common:jar:3.5.0-SNAPSHOT in Nexus (http://repository.apache.org/snapshots) at org.eclipse.aether.internal.impl.DefaultArtifactResolver.resolve(DefaultArtifactResolver.java:444) at org.eclipse.aether.internal.impl.DefaultArtifactResolver.resolveArtifacts(DefaultArtifactResolver.java:246) at org.eclipse.aether.internal.impl.DefaultRepositorySystem.resolveDependencies(DefaultRepositorySystem.java:367) ... 36 moreCaused by: org.eclipse.aether.transfer.ArtifactNotFoundException: Could not find artifact org.apache.apex:apex-common:jar:3.5.0-SNAPSHOT in Nexus (http://repository.apache.org/snapshots) at org.eclipse.aether.connector.basic.ArtifactTransportListener.transferFailed(ArtifactTransportListener.java:39) at org.eclipse.aether.connector.basic.BasicRepositoryConnector$TaskRunner.run(BasicRepositoryConnector.java:355) at org.eclipse.aether.util.concurrency.RunnableErrorForwarder$1.run(RunnableErrorForwarder.java:67) ... 3 more2017-01-20T13:12:27.925 [ERROR] 2017-01-20T13:12:27.925 [ERROR] Re-run Maven using the -X switch to enable full debug logging.2017-01-20T13:12:27.925 [ERROR] 2017-01-20T13:12:27.925 [ERROR] For more information about the errors and possible solutions, please read the following articles:2017-01-20T13:12:27.925 [ERROR] [Help 1] http://cwiki.apache.org/confluence/display/MAVEN/DependencyResolutionException2017-01-20T13:12:27.925 [ERROR] 2017-01-20T13:12:27.925 [ERROR] After correcting the problems, you can resume the build with the command2017-01-20T13:12:27.925 [ERROR] mvn -rf :beam-runners-apexchannel stoppedSetting status of e3a1d73 to FAILURE with url https://builds.apache.org/job/beam_PreCommit_Java_MavenInstall/6694/ and message: 'Build finished. 'Using context: Jenkins: Maven clean install--none-- |
…lements duplication
Rebased and added a test to illustrate the expected behavior. |
Refer to this link for build results (access rights to CI server needed): |
LGTM, thanks JB! |
Be sure to do all of the following to help us incorporate your contribution
quickly and easily:
[BEAM-<Jira issue #>] Description of pull request
mvn clean verify
. (Even better, enableTravis-CI on your fork and ensure the whole test matrix passes).
<Jira issue #>
in the title with the actual Jira issuenumber, if there is one.
Individual Contributor License Agreement.
…otential messages duplication