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

allow duplicate stream titles in route_to_stream #154

Merged
merged 4 commits into from Jan 10, 2017
Merged

allow duplicate stream titles in route_to_stream #154

merged 4 commits into from Jan 10, 2017

Conversation

@kroepke
Copy link
Member

@kroepke kroepke commented Jan 3, 2017

cache stream ids and titles to avoid heavy database traffic during function evaluation

fixes #101

cache stream ids and titles to avoid heavy database traffic during function evaluation

fixes #101
@kroepke kroepke added this to the 2.2.0 milestone Jan 3, 2017
@kroepke kroepke self-assigned this Jan 3, 2017
this avoids flapping between streams by explicitly ordering them in the cache
@kroepke kroepke changed the title [WIP] allow duplicate stream titles in route_to_stream allow duplicate stream titles in route_to_stream Jan 4, 2017
@kroepke kroepke removed their assignment Jan 5, 2017
@joschi joschi self-assigned this Jan 5, 2017
@@ -98,6 +100,8 @@ protected void configure() {
addMessageProcessorFunction(DropMessage.NAME, DropMessage.class);
addMessageProcessorFunction(CreateMessage.NAME, CreateMessage.class);
addMessageProcessorFunction(RouteToStream.NAME, RouteToStream.class);
// helper service for route_to_stream
serviceBinder().addBinding().to(StreamCacheService.class).in(Scopes.SINGLETON);

This comment has been minimized.

@joschi

joschi Jan 5, 2017
Contributor

Isn't using the @Singleton annotation in StreamCacheService sufficient?

This comment has been minimized.

@kroepke

kroepke Jan 5, 2017
Author Member

I'm not sure, tbh. I like to have bindings explicit and would even disable on-the-fly discovery, if it was up to me.

This comment has been minimized.

@joschi

joschi Jan 10, 2017
Contributor

}

@VisibleForTesting
public void updateStreams(ImmutableSet<String> ids) {

This comment has been minimized.

@joschi

joschi Jan 5, 2017
Contributor

Aesthetic nitpick: The method argument could be Set<String> or Collection<String>.

}


public Stream getByName(String name) {

This comment has been minimized.

@joschi

joschi Jan 5, 2017
Contributor

This method should be marked as @Nullable.

return Iterables.getFirst(nameToStream.get(name), null);
}

public Stream getById(String id) {

This comment has been minimized.

@joschi

joschi Jan 5, 2017
Contributor

This method should be marked as @Nullable.

import com.google.common.collect.Iterables;
import com.google.common.collect.Lists;
import com.google.common.collect.Sets;
import com.google.common.eventbus.EventBus;
import com.google.common.net.InetAddresses;

import com.fasterxml.jackson.databind.ObjectMapper;

This comment has been minimized.

@joschi

joschi Jan 5, 2017
Contributor

Aestehtic nitpick: Stray newlines?

This comment has been minimized.

@kroepke

kroepke Jan 5, 2017
Author Member

this is auto-optimized (google code style), but since it's just an import in a test, I'll leave it

This comment has been minimized.

@joschi

joschi Jan 5, 2017
Contributor

when(streamService.load(ArgumentMatchers.eq("id"))).thenReturn(stream);
when(streamService.load(ArgumentMatchers.eq("id2"))).thenReturn(stream2);
} catch (NotFoundException ignored) {
// oh well, checked exceptions <3

This comment has been minimized.

@joschi

joschi Jan 5, 2017
Contributor

Would adding it to the method declaration via throws help to get rid of this try block?

This comment has been minimized.

@kroepke

kroepke Jan 5, 2017
Author Member

dito, test artefact



public Stream getByName(String name) {
return Iterables.getFirst(nameToStream.get(name), null);

This comment has been minimized.

@joschi

joschi Jan 5, 2017
Contributor

This generally opens the question, if route_to_stream(name: "foo") should route the message into all streams named "foo" or just into the first one.

As a user, I'd probably expect the message to be routed into all of these streams.

This comment has been minimized.

@kroepke

kroepke Jan 5, 2017
Author Member

Hmm, that would be a viable option, yes.
I'm on the fence, because I feel that duplicate stream names are a bug in themselves, but taking the first one is probably not the best idea either.

This comment has been minimized.

@edmundoa

edmundoa Jan 5, 2017
Member

I agree using several streams with the same name is confusing and a bad practice, although I would also expect that the message would go to all streams with the given name.

This comment has been minimized.

@kroepke

kroepke Jan 5, 2017
Author Member

Ok, I'll change it accordingly.


private final EventBus eventBus;
private final StreamService streamService;
private ScheduledExecutorService executorService;

This comment has been minimized.

@joschi

joschi Jan 5, 2017
Contributor

Should be final.

private final StreamService streamService;
private ScheduledExecutorService executorService;

private SortedSetMultimap<String, Stream> nameToStream = Multimaps.synchronizedSortedSetMultimap(

This comment has been minimized.

@joschi

joschi Jan 5, 2017
Contributor

Should be final.

MultimapBuilder.hashKeys()
.treeSetValues(Comparator.comparing(Stream::getId))
.build());
private Map<String, Stream> idToStream = Maps.newConcurrentMap();

This comment has been minimized.

@joschi

joschi Jan 5, 2017
Contributor

Should be final.

@joschi
joschi approved these changes Jan 10, 2017
@@ -98,6 +100,8 @@ protected void configure() {
addMessageProcessorFunction(DropMessage.NAME, DropMessage.class);
addMessageProcessorFunction(CreateMessage.NAME, CreateMessage.class);
addMessageProcessorFunction(RouteToStream.NAME, RouteToStream.class);
// helper service for route_to_stream
serviceBinder().addBinding().to(StreamCacheService.class).in(Scopes.SINGLETON);

This comment has been minimized.

@joschi

joschi Jan 10, 2017
Contributor

@joschi joschi merged commit 7c7482a into master Jan 10, 2017
3 checks passed
3 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details
@joschi joschi deleted the issue-101 branch Jan 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants