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

KAFKA-10316: Updated Kafka Streams upgrade notes. #9133

Closed
wants to merge 5 commits into from
Closed

KAFKA-10316: Updated Kafka Streams upgrade notes. #9133

wants to merge 5 commits into from

Conversation

johnthotekat
Copy link
Contributor

@johnthotekat johnthotekat commented Aug 7, 2020

johnthotekat and others added 5 commits August 4, 2020 11:39
* Deprecated the existing getters and added new getters `activeHost()`, `standbyHosts()`, and `partition()` to KeyQueryMetadata.java.

* Update the existing Tests to use the new getters of KeyQueryMetadata.java . Below are the Test classes that were updated with new getters.

    OptimizedKTableIntegrationTest.java
    QueryableStateIntegrationTest.java
    StoreQueryIntegrationTest.java
    StreamsMetadataStateTest.java
* Deprecated the existing getters and added new getters `activeHost()`, `standbyHosts()`, and `partition()` to KeyQueryMetadata.java.

* Update the existing Tests to use the new getters of KeyQueryMetadata.java . Below are the Test classes that were updated with new getters.

    OptimizedKTableIntegrationTest.java
    QueryableStateIntegrationTest.java
    StoreQueryIntegrationTest.java
    StreamsMetadataStateTest.java

* Correcting some JavaDoc nits.
Merge apache/kafka to johnthotekat/kafka
Adding KIP-648 documentations to streams upgrade notes.
@johnthotekat
Copy link
Contributor Author

@mjsax I've checked the actual documentations and just to make sure I've also checked for the occurrence of the deprecated methods throughout the repo and found none (except in the classes and upgrade-guide which we modified).

@@ -50,32 +50,65 @@ public KeyQueryMetadata(final HostInfo activeHost, final Set<HostInfo> standbyHo
}

/**
* Get the Active streams instance for given key
Copy link
Member

Choose a reason for hiding this comment

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

As the original PR is merged already, this seems to be redundant (similar below for all other code changes). -- This PR should only update the html file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah , this should only update the html file. I raised the PR from the same branch. Err., I wonder, as these changes were already merged, should it even be showing these as changes here ? I'm saying about the previous changes which were already merged.

Copy link
Contributor Author

@johnthotekat johnthotekat Aug 7, 2020

Choose a reason for hiding this comment

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

If you see from the commit history in the timeline . My last commit is : 14ab052 (this has only the html changes) and all the ones before that are already merged to trunk. Am I missing something here for the previous changes to show up in this PR too?

Copy link
Member

Choose a reason for hiding this comment

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

Not sure. I am not an git guru... Can you maybe just rebase this PR against latest trunk (just to make sure we don't mess anything up?)

@johnthotekat
Copy link
Contributor Author

johnthotekat commented Aug 8, 2020

Closing this PR. Raised another PR #9146 .
This branch got a little messed up during the rebase :(

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants