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

fix regression in app id creation #498

Merged
merged 2 commits into from
Dec 6, 2017
Merged

fix regression in app id creation #498

merged 2 commits into from
Dec 6, 2017

Conversation

dguy
Copy link
Contributor

@dguy dguy commented Dec 4, 2017

The app id for persistent queries shouldn't have a time stamp suffix applied otherwise it will result in different app ids across different servers.

Copy link
Contributor

@apurvam apurvam left a comment

Choose a reason for hiding this comment

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

How hard would it be to add a unit test for this?

String applicationId = serviceId + persistanceQueryPrefix + queryId;
if (addUniqueTimeSuffix) {
applicationId = addTimeSuffix(applicationId);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Actually addUniqueTimeSuffix will always be false here and will always be true for buildPlanForBareQuery method. Maybe we should remove this boolean all together.

Copy link
Contributor

@apurvam apurvam left a comment

Choose a reason for hiding this comment

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

LGTM. Still think it would be nice to start adding fine grained unit tests for these things if it isn't too hard.

Copy link
Contributor

@hjafarpour hjafarpour left a comment

Choose a reason for hiding this comment

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

LGTM.

@dguy dguy merged commit 3115912 into confluentinc:master Dec 6, 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
Development

Successfully merging this pull request may close these issues.

3 participants