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

Persistent streams #3030

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Persistent streams #3030

wants to merge 11 commits into from

Conversation

MGathier
Copy link
Member

@MGathier MGathier commented Jun 10, 2024

Depends on AxonIQ/axonserver-connector-java#347 to be merged.

@MGathier MGathier added this to the Release 4.10.0 milestone Jun 10, 2024
@MGathier MGathier self-assigned this Jun 10, 2024
Copy link
Member

@gklijs gklijs left a comment

Choose a reason for hiding this comment

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

Mostly javadoc nits, looks nice. Also prefer to add the released connector version, as soon as it's there before merging.

@@ -1206,6 +1213,14 @@ public void setEventStoreConfiguration(EventStoreConfiguration eventStoreConfigu
this.eventStoreConfiguration = eventStoreConfiguration;
}

public int getPersistentStreamThreads() {
Copy link
Member

Choose a reason for hiding this comment

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

Needs javadoc

return persistentStreamThreads;
}

public void setPersistentStreamThreads(int persistentStreamThreads) {
Copy link
Member

Choose a reason for hiding this comment

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

Needs javadoc

persistentStreamHolder.set(null);
if (throwable != null) {
logger.info("{}: Rescheduling persistent stream", streamId, throwable);
scheduler.schedule(this::start, 1, TimeUnit.SECONDS);
Copy link
Member

Choose a reason for hiding this comment

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

Maybe this would be better with an incremental time? A lot of applications at the same time might be able to bring Axon Server down, keeping it going in circles?

}
return metaDataValues;
}
if ("SequentialPolicy".equals(settings.getSequencingPolicy())) {
Copy link
Member

@gklijs gklijs Jun 10, 2024

Choose a reason for hiding this comment

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

I think this is correct, but it does mean we should be weary of the combination of sequential policy with the dead letter queue, as it could fill up quickly.

}

@Test
void open() {
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer a clearer test name, maybe something like canOpenStreamAndProcessTwoEvents or something similar.

@@ -71,7 +71,7 @@
${project.basedir}/../coverage-report/target/site/jacoco-aggregate/jacoco.xml
</sonar.coverage.jacoco.xmlReportPaths>
<!-- Axon Server -->
<axonserver-connector-java.version>2023.2.0</axonserver-connector-java.version>
<axonserver-connector-java.version>2024.0.0-SNAPSHOT</axonserver-connector-java.version>
Copy link
Member

Choose a reason for hiding this comment

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

We should remember to put in a released version, at the latest before releasing 4.10 of framework. Ideally through it's better to change it to the released version as soon as it's available, still in this pr.

Copy link
Member

Choose a reason for hiding this comment

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

The release plugin will automatically detect and resolve dependencies pointing at snapshots.

this(eventProcessorProperties, null, deadLetterQueueProvider);
}

public DeadLetterQueueProviderConfigurerModule(
Copy link
Member

Choose a reason for hiding this comment

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

Needs javadoc

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.

None yet

4 participants