Adding in the PubSub package. #1
Conversation
|
Thank you for submitting this pull request, however I do not see a valid CLA on file for you. Before we can merge this request please visit https://yahoocla.herokuapp.com/ and agree to the terms. Thanks! 😄 |
pom.xml
Outdated
| <groupId>com.yahoo.bullet</groupId> | ||
| <artifactId>bullet-core</artifactId> | ||
| <version>0.1.3-SNAPSHOT</version> | ||
| <version>0.2.0</version> |
There was a problem hiding this comment.
It should be 0.2.0-SNAPSHOT. The release process strips it off when releasing.
| @@ -0,0 +1,55 @@ | |||
| package com.yahoo.bullet.pubsub; | |||
|
|
|||
| public abstract class Subscriber { | |||
There was a problem hiding this comment.
Use interface and default methods
| private String id; | ||
| private String content; | ||
| private Metadata metadata; | ||
| private long sequenceNumber; |
There was a problem hiding this comment.
Use int, call it just sequence (it's clear it's a number) and can you move this to right after id?
| * @param sequenceNumber is the sequence number associated with the message. | ||
| * @param metadata is the {@link Metadata} associated with the message. | ||
| */ | ||
| public PubSubMessage(String id, String content, long sequenceNumber, Metadata metadata) { |
There was a problem hiding this comment.
All constructors should just call this one with the right arguments
| /** | ||
| * Constructor for a message having only content. | ||
| * | ||
| * @param id is the query ID associated with the message. |
There was a problem hiding this comment.
We shouldn't mention that this is the queryID everywhere. I think we should just a class javadoc that mentions how PubSubMessage can be used with id being a query id and sequence being multi-part atomic messages per query.
| String messageContent = getRandomString(); | ||
| String metadataContent = getRandomString(); | ||
| Signal signal = Signal.ACKNOWLEDGE; | ||
| //Test creation with a sequence number. |
There was a problem hiding this comment.
Break it up into another test?
| Assert.assertNotNull(message.getMetadata()); | ||
| Assert.assertEquals(message.getMetadata().getSignal(), signal); | ||
| Assert.assertTrue(message.getMetadata().getContent().toString().equals(metadataContent)); | ||
| //Test creation without a sequence number. |
| @Test | ||
| public void testCommitWithNoSequenceNumber() { | ||
| String randomID = UUID.randomUUID().toString(); | ||
| Subscriber subscriber = Mockito.mock(Subscriber.class, Mockito.CALLS_REAL_METHODS); |
There was a problem hiding this comment.
If you test it, you don't have to mock it. Just create a minimal test implementation (can be in this file) and just call the default method if you need it.
|
Ready for re-review. |
5 participants
No description provided.