[fix][functions] Ensure InternalConfigurationData data model is compatible across different versions - #17690
Merged
michaeljmarshall merged 3 commits intoSep 16, 2022
Conversation
…tible across different versions
nicoloboschi
added
area/function
area/admin
release/2.11.0
release/2.10.3
release/blocker
nicoloboschi
requested review from
Jason918,
Technoboy-,
eolivelli,
gaozhangmin,
lhotari,
michaeljmarshall and
nodece
September 16, 2022 08:45
nodece
approved these changes
Sep 16, 2022
codelipenghui
approved these changes
Sep 16, 2022
michaeljmarshall
approved these changes
Sep 16, 2022
michaeljmarshall
left a comment
michaeljmarshall
left a comment
Member
There was a problem hiding this comment.
LGTM. The PR description was very helpful, thanks!
michaeljmarshall
pushed a commit
that referenced
this pull request
Sep 16, 2022
…tible across different versions (#17690) * [fix][functions] Ensure InternalConfigurationData data model is compatible across different versions * style * fix the other way After #14384, the broker and the client expects that the `InternalConfigurationData` contains `metadataStoreUrl` and `configurationMetadataStoreUrl` fields. However the broker is no more compatible with old clients. #14384 is landed to branch-2.11 and [2.10.2](#17291) Example scenario: - broker on 2.10.1 - function worker on 2.10.1 1. upgrade fn worker to 2.11.0 or 2.10.2 2. the fn worker starts and download the internal config from the broker 3. broker serves a json with old fields (`zookeeperServers` and `configurationStoreServers`) 4. fn worker reads the json and convert it to a `InternalConfigurationData` instance. It expects to see the fields filled `metadataStoreUrl` and `configurationMetadataStoreUrl` but they aren't 5. NPE on fn worker ``` 2022-09-15T17:42:16,072+0000 [main] INFO org.apache.pulsar.functions.worker.PulsarWorkerService - Initializing Pulsar Functions namespace... 2022-09-15T17:42:16,192+0000 [main] ERROR org.apache.pulsar.functions.worker.FunctionWorkerStarter - Encountered error in function worker. java.lang.NullPointerException: null at org.apache.pulsar.metadata.impl.MetadataStoreFactoryImpl.removeIdentifierFromMetadataURL(MetadataStoreFactoryImpl.java:73) at org.apache.pulsar.functions.worker.WorkerUtils.initializeDlogNamespace(WorkerUtils.java:188) at org.apache.pulsar.functions.worker.PulsarWorkerService.initializeStandaloneWorkerService(PulsarWorkerService.java:281) at org.apache.pulsar.functions.worker.PulsarWorkerService.initAsStandalone(PulsarWorkerService.java:208) at org.apache.pulsar.functions.worker.Worker.start(Worker.java:54) at org.apache.pulsar.functions.worker.FunctionWorkerStarter.main(FunctionWorkerStarter.java:76) ``` Additionaly there's the same issue if we upgrade the broker before the fn worker: 1. the broker gets the upgrade. won't serve `zookeeperServers` field 2. fn worker restarts for some reasons. 3. fn worker gets the internal config and look for `zookeeperServers` field which is empty in the json 4. NPE * Restore old fields in `InternalConfigurationData` and add fallback the old values in the new fields getters * Added unit test - [x] `doc-not-needed` (cherry picked from commit 1a34b87)
michaeljmarshall
pushed a commit
that referenced
this pull request
Sep 16, 2022
…tible across different versions (#17690) * [fix][functions] Ensure InternalConfigurationData data model is compatible across different versions * style * fix the other way ### Motivation After #14384, the broker and the client expects that the `InternalConfigurationData` contains `metadataStoreUrl` and `configurationMetadataStoreUrl` fields. However the broker is no more compatible with old clients. #14384 is landed to branch-2.11 and [2.10.2](#17291) Example scenario: - broker on 2.10.1 - function worker on 2.10.1 1. upgrade fn worker to 2.11.0 or 2.10.2 2. the fn worker starts and download the internal config from the broker 3. broker serves a json with old fields (`zookeeperServers` and `configurationStoreServers`) 4. fn worker reads the json and convert it to a `InternalConfigurationData` instance. It expects to see the fields filled `metadataStoreUrl` and `configurationMetadataStoreUrl` but they aren't 5. NPE on fn worker ``` 2022-09-15T17:42:16,072+0000 [main] INFO org.apache.pulsar.functions.worker.PulsarWorkerService - Initializing Pulsar Functions namespace... 2022-09-15T17:42:16,192+0000 [main] ERROR org.apache.pulsar.functions.worker.FunctionWorkerStarter - Encountered error in function worker. java.lang.NullPointerException: null at org.apache.pulsar.metadata.impl.MetadataStoreFactoryImpl.removeIdentifierFromMetadataURL(MetadataStoreFactoryImpl.java:73) at org.apache.pulsar.functions.worker.WorkerUtils.initializeDlogNamespace(WorkerUtils.java:188) at org.apache.pulsar.functions.worker.PulsarWorkerService.initializeStandaloneWorkerService(PulsarWorkerService.java:281) at org.apache.pulsar.functions.worker.PulsarWorkerService.initAsStandalone(PulsarWorkerService.java:208) at org.apache.pulsar.functions.worker.Worker.start(Worker.java:54) at org.apache.pulsar.functions.worker.FunctionWorkerStarter.main(FunctionWorkerStarter.java:76) ``` Additionaly there's the same issue if we upgrade the broker before the fn worker: 1. the broker gets the upgrade. won't serve `zookeeperServers` field 2. fn worker restarts for some reasons. 3. fn worker gets the internal config and look for `zookeeperServers` field which is empty in the json 4. NPE ### Modifications * Restore old fields in `InternalConfigurationData` and add fallback the old values in the new fields getters * Added unit test - [x] `doc-not-needed` (cherry picked from commit 1a34b87)
nicoloboschi
added a commit
to datastax/pulsar
that referenced
this pull request
Sep 20, 2022
…tible across different versions (apache#17690) * [fix][functions] Ensure InternalConfigurationData data model is compatible across different versions * style * fix the other way After apache#14384, the broker and the client expects that the `InternalConfigurationData` contains `metadataStoreUrl` and `configurationMetadataStoreUrl` fields. However the broker is no more compatible with old clients. apache#14384 is landed to branch-2.11 and [2.10.2](apache#17291) Example scenario: - broker on 2.10.1 - function worker on 2.10.1 1. upgrade fn worker to 2.11.0 or 2.10.2 2. the fn worker starts and download the internal config from the broker 3. broker serves a json with old fields (`zookeeperServers` and `configurationStoreServers`) 4. fn worker reads the json and convert it to a `InternalConfigurationData` instance. It expects to see the fields filled `metadataStoreUrl` and `configurationMetadataStoreUrl` but they aren't 5. NPE on fn worker ``` 2022-09-15T17:42:16,072+0000 [main] INFO org.apache.pulsar.functions.worker.PulsarWorkerService - Initializing Pulsar Functions namespace... 2022-09-15T17:42:16,192+0000 [main] ERROR org.apache.pulsar.functions.worker.FunctionWorkerStarter - Encountered error in function worker. java.lang.NullPointerException: null at org.apache.pulsar.metadata.impl.MetadataStoreFactoryImpl.removeIdentifierFromMetadataURL(MetadataStoreFactoryImpl.java:73) at org.apache.pulsar.functions.worker.WorkerUtils.initializeDlogNamespace(WorkerUtils.java:188) at org.apache.pulsar.functions.worker.PulsarWorkerService.initializeStandaloneWorkerService(PulsarWorkerService.java:281) at org.apache.pulsar.functions.worker.PulsarWorkerService.initAsStandalone(PulsarWorkerService.java:208) at org.apache.pulsar.functions.worker.Worker.start(Worker.java:54) at org.apache.pulsar.functions.worker.FunctionWorkerStarter.main(FunctionWorkerStarter.java:76) ``` Additionaly there's the same issue if we upgrade the broker before the fn worker: 1. the broker gets the upgrade. won't serve `zookeeperServers` field 2. fn worker restarts for some reasons. 3. fn worker gets the internal config and look for `zookeeperServers` field which is empty in the json 4. NPE * Restore old fields in `InternalConfigurationData` and add fallback the old values in the new fields getters * Added unit test - [x] `doc-not-needed` (cherry picked from commit 1a34b87) (cherry picked from commit 0993a34)
Contributor
|
@nicoloboschi |
Contributor
Author
|
@Jason918 I pushed a fix for the test. The motivation is that this commit is not present in 2.10. But it's not a problem, I just updated the test. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
After #14384, the broker and the client expects that the
InternalConfigurationDatacontainsmetadataStoreUrlandconfigurationMetadataStoreUrlfields.However the broker is no more compatible with old clients.
#14384 is landed to branch-2.11 and 2.10.2
Example scenario:
zookeeperServersandconfigurationStoreServers)InternalConfigurationDatainstance. It expects to see the fields filledmetadataStoreUrlandconfigurationMetadataStoreUrlbut they aren'tAdditionaly there's the same issue if we upgrade the broker before the fn worker:
zookeeperServersfieldzookeeperServersfield which is empty in the jsonModifications
InternalConfigurationDataand add fallback the old values in the new fields gettersdoc-not-needed