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

Sourcerer2: Moving to string based versions, introducing JDBC driver #22

Open
wants to merge 56 commits into
base: sourcerer2
Choose a base branch
from

Conversation

kristian-elder
Copy link
Contributor

@kristian-elder kristian-elder commented Dec 10, 2018

This change is Reviewable


/**
* Throw to indicate that the current version of a stream does not match the expected one.
*/
public class UnexpectedVersionException extends IllegalStateException {
private final Integer currentVersion;
private final StreamVersion currentVersion;

Choose a reason for hiding this comment

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

CRITICAL Make "currentVersion" transient or serializable. rule


/**
* Throw to indicate that the current version of a stream does not match the expected one.
*/
public class UnexpectedVersionException extends IllegalStateException {
private final Integer currentVersion;
private final StreamVersion currentVersion;
private final ExpectedVersion expectedVersion;

Choose a reason for hiding this comment

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

CRITICAL Make "expectedVersion" transient or serializable. rule

}

private fun serializeMetadata(metadata: Map<String, String>): String {
// TODO: Handle errors more nicely

Choose a reason for hiding this comment

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

INFO Flags a forbidden comment. Defaults values are TODO:, FIXME: or STOPSHIP: rule

import org.reactivestreams.Publisher
import org.slf4j.LoggerFactory

internal class DbstoreEventRepository<T>(
Copy link

@ci-elder ci-elder Dec 14, 2018

Choose a reason for hiding this comment

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

MAJOR Classes with many functions tend to do too many things and often come in conjunction with large classes and can quickly become God classes. Consider extracting methods to (new) classes better matching their responsibility. SIZE 15 is greater than the threshold 10. rule

}

private fun serializeEvent(event: T): String {
// TODO: Handle errors more nicely

Choose a reason for hiding this comment

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

INFO Flags a forbidden comment. Defaults values are TODO:, FIXME: or STOPSHIP: rule

}

private fun parseEvent(data: String): T {
// TODO: Handle errors more nicely

Choose a reason for hiding this comment

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

INFO Flags a forbidden comment. Defaults values are TODO:, FIXME: or STOPSHIP: rule

}

private fun parseMetadata(metadata: String): ImmutableMap<String, String> {
// TODO: Handle errors more nicely

Choose a reason for hiding this comment

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

INFO Flags a forbidden comment. Defaults values are TODO:, FIXME: or STOPSHIP: rule

streamId: StreamId,
category: String,
fromVersion: DbstoreStreamVersion? = null,
maxEvents: Int = 4096

Choose a reason for hiding this comment

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

INFO Report magic numbers. Magic number is a numeric literal that is not defined as a constant and hence it's unclear what the purpose of this number is. It's better to declare such numbers as constants and give them a proper name. By default, -1, 0, 1, and 2 are not considered to be magic numbers. rule

category: String,
shard: Int?,
fromVersion: DbstoreRepositoryVersion? = null,
maxEvents: Int = 4096

Choose a reason for hiding this comment

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

INFO Report magic numbers. Magic number is a numeric literal that is not defined as a constant and hence it's unclear what the purpose of this number is. It's better to declare such numbers as constants and give them a proper name. By default, -1, 0, 1, and 2 are not considered to be magic numbers. rule

return "ExpectedVersion.AnyExisting"
}
}
object Any : ExpectedVersion() {

Choose a reason for hiding this comment

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

INFO MultipleSpaces rule

+ "' with 'not created'",
baseVersion, newVersion)
else ->
throw RuntimeException(

Choose a reason for hiding this comment

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

CRITICAL Caught exception is too generic. Prefer catching specific Exceptions to handle specific error cases. rule

ExpectedVersion.Any -> 0
ExpectedVersion.AnyExisting -> 1
is ExpectedVersion.Exactly -> 2
ExpectedVersion.NotCreated -> 3

Choose a reason for hiding this comment

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

INFO Report magic numbers. Magic number is a numeric literal that is not defined as a constant and hence it's unclear what the purpose of this number is. It's better to declare such numbers as constants and give them a proper name. By default, -1, 0, 1, and 2 are not considered to be magic numbers. rule

ExpectedVersion.NotCreated -> when (newVersion) {
ExpectedVersion.NotCreated -> baseVersion
else ->
throw RuntimeException(

Choose a reason for hiding this comment

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

CRITICAL Caught exception is too generic. Prefer catching specific Exceptions to handle specific error cases. rule

} else baseVersion ?: (newVersion ?: ExpectedVersion.any())
}

/**

Choose a reason for hiding this comment

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

MINOR Comments for private functions should be avoided. Prefer giving the function an expressive name. Split it up in smaller, self-explaining functions if necessary. rule

return "ExpectedVersion.NotCreated"
}
}
object AnyExisting : ExpectedVersion() {

Choose a reason for hiding this comment

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

INFO MultipleSpaces rule

throw IllegalArgumentException("Version is not in expected format")
}

val timestampStr = version.substring(0, 24)

Choose a reason for hiding this comment

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

INFO Report magic numbers. Magic number is a numeric literal that is not defined as a constant and hence it's unclear what the purpose of this number is. It's better to declare such numbers as constants and give them a proper name. By default, -1, 0, 1, and 2 are not considered to be magic numbers. rule


sealed class EventSubscriptionUpdate<T> {
class CaughtUp<T> : EventSubscriptionUpdate<T>()
data class Event<T>(val event: EventRecord<T>): EventSubscriptionUpdate<T>()

Choose a reason for hiding this comment

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

INFO SpacingAroundColon rule


import java.time.Instant;
import java.util.List;
import java.util.UUID;
import java.util.stream.IntStream;

import static org.mockito.Matchers.any;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;

public class EventSubscriptionManagerTest {

Choose a reason for hiding this comment

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

MAJOR Add some tests to this class. rule

@ci-elder
Copy link

SonarQube analysis reported 52 issues

  • CRITICAL 6 critical
  • MAJOR 11 major
  • MINOR 6 minor
  • INFO 29 info

Watch the comments in this conversation to review them.

7 extra issues

Note: The following issues were found on lines that were not modified in the pull request. Because these issues can't be reported as line comments, they are summarized here:

  1. CRITICAL CommandUtils.java#L184: Either re-interrupt this method or rethrow the "InterruptedException". rule
  2. MAJOR CommandResult.java#L52: Remove usage of generic wildcard type. rule
  3. MAJOR DefaultImmutableAggregate.java#L279: Rename "state" which hides the field declared at line 35. rule
  4. MAJOR DefaultImmutableAggregate.java#L290: Rename "state" which hides the field declared at line 35. rule
  5. MAJOR DefaultImmutableAggregate.java#L301: Rename "state" which hides the field declared at line 35. rule
  6. MAJOR DefaultImmutableAggregate.java#L312: Rename "state" which hides the field declared at line 35. rule
  7. MAJOR DefaultImmutableAggregate.java#L323: Rename "state" which hides the field declared at line 35. rule

kristian-elder and others added 27 commits January 7, 2019 16:09
That way we know it doesn't blow up when it doesn't exist.
To make the options explicit.
Allow for noop:ing a create if aggregate already exists
Prepare for adding retry-support.
If an update was made to the aggregate after it was read but before
it was updated, retry (according to policy).
Don't say "current version null" when current version is unknown, as it
looks like current version is notExisting.

Keep UnexpectedVersionException in stack trace for AtomicWriteException.
Instead use RetryHandler and RetryPolicy directly.
Don't add variance.
Add support for retrying atomic write failures
Which supports preferRandomNode
…store-health-probe

Prometheus monitoring for EventStore health probe
Upgrading to ESJC 2.x (with 64 bit positions)
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.

4 participants