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

Primary to Secondary Mapper #1300

Merged
merged 12 commits into from
Jun 24, 2022
Merged

Primary to Secondary Mapper #1300

merged 12 commits into from
Jun 24, 2022

Conversation

csviri
Copy link
Collaborator

@csviri csviri commented Jun 23, 2022

see related issue

@csviri csviri self-assigned this Jun 23, 2022
@csviri csviri linked an issue Jun 23, 2022 that may be closed by this pull request
@csviri csviri marked this pull request as ready for review June 23, 2022 15:32
@csviri csviri requested a review from metacosm June 23, 2022 19:10
primaryToSecondaryIndex =
new PrimaryToSecondaryIndex<>(configuration.getSecondaryToPrimaryMapper());
} else {
primaryToSecondaryIndex = null;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe it'd be cleaner to have a no-op index instead of setting it to null and having to guard for null everywhere else?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not sure about that, it is very explicit this way, that the index is not used in that case.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

But also fine with that. Will try how that looks.

Copy link
Collaborator Author

@csviri csviri Jun 24, 2022

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

One doesn't prevent the other: we could have a NOOP index and still be able to make it explicit when needed (i.e. useSecondaryToPrimaryIndex could check if the set index is the NOOP one instead of null where we need to know for sure whether to use it or not). Not a showstopper in any case.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Changed.

@morhidi
Copy link

morhidi commented Jun 24, 2022

Attila, thank you very much for the prompt fix, I have tested this PR locally. I can confirm this addresses the issue we hit in FLINK-28008. Do you already have an ETA for the 3.0.3 patch release?

@csviri
Copy link
Collaborator Author

csviri commented Jun 24, 2022

Attila, thank you very much for the prompt fix, I have tested this PR locally. I can confirm this addresses the issue we hit in FLINK-28008. Do you already have an ETA for the 3.0.3 patch release?

@morhidi thx for feedback, glad to hear, after this issue is merged we can release v3.0.3, hopefully today or Monday.

@csviri csviri requested a review from metacosm June 24, 2022 11:05
@sonarcloud
Copy link

sonarcloud bot commented Jun 24, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

43.2% 43.2% Coverage
0.0% 0.0% Duplication

@csviri csviri merged commit ab111e7 into main Jun 24, 2022
@csviri csviri deleted the primary-to-secondary branch June 24, 2022 13:14
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.

Cannot get secondary resource from context after operator restart
3 participants