chore: Update airlift to 0.227 version#27070
Conversation
Reviewer's GuideAdds a configurable feature flag to disable Jackson’s INTERN_FIELD_NAMES during JSON deserialization and wires it into server bootstrap so high‑concurrency paths can use a custom ObjectMapper that avoids String intern cache contention, with corresponding configuration tests. Sequence diagram for conditional ObjectMapper binding based on jsonInternFieldNamesDisabledsequenceDiagram
participant ConfigLoader
participant FeaturesConfig
participant ServerMainModule
participant GuiceBinder as GuiceBinder
participant PrestoJsonObjectMapperProvider
participant JsonObjectMapperProvider
participant ClientComponent
ConfigLoader->>FeaturesConfig: construct with json_intern_field_names_disabled
Note over FeaturesConfig: jsonInternFieldNamesDisabled set from config
ServerMainModule->>FeaturesConfig: isJsonInternFieldNamesDisabled()
FeaturesConfig-->>ServerMainModule: boolean disabled
alt intern field names disabled
ServerMainModule->>GuiceBinder: bind ObjectMapper to PrestoJsonObjectMapperProvider
else intern field names enabled
ServerMainModule->>GuiceBinder: bind ObjectMapper to JsonObjectMapperProvider
end
ClientComponent->>GuiceBinder: request ObjectMapper
alt disabled
GuiceBinder->>PrestoJsonObjectMapperProvider: get()
PrestoJsonObjectMapperProvider-->>GuiceBinder: ObjectMapper(INTERN_FIELD_NAMES disabled)
else enabled
GuiceBinder->>JsonObjectMapperProvider: get()
JsonObjectMapperProvider-->>GuiceBinder: ObjectMapper(default settings)
end
GuiceBinder-->>ClientComponent: ObjectMapper instance
Class diagram for new JSON intern-field-names configuration and ObjectMapper providerclassDiagram
class FeaturesConfig {
- boolean skipPushdownThroughExchangeForRemoteProjection
- String remoteFunctionNamesForFixedParallelism
- int remoteFunctionFixedParallelismTaskCount
- boolean jsonInternFieldNamesDisabled
+ FeaturesConfig setRemoteFunctionFixedParallelismTaskCount(int remoteFunctionFixedParallelismTaskCount)
+ FeaturesConfig setJsonInternFieldNamesDisabled(boolean jsonInternFieldNamesDisabled)
+ boolean isJsonInternFieldNamesDisabled()
}
class ServerMainModule {
+ ListeningExecutorService createResourceManagerExecutor(ResourceManagerConfig resourceManagerConfig)
..ObjectMapper binding..
}
class ObjectMapperProvider {
<<framework>>
+ ObjectMapperProvider()
+ ObjectMapperProvider(JsonFactory jsonFactory)
+ ObjectMapper get()
}
class JsonObjectMapperProvider {
<<framework>>
+ JsonObjectMapperProvider()
+ ObjectMapper get()
}
class PrestoJsonObjectMapperProvider {
+ PrestoJsonObjectMapperProvider()
+ ObjectMapper get()
}
class JsonFactoryBuilder {
+ JsonFactoryBuilder()
+ JsonFactoryBuilder disable(Feature feature)
+ JsonFactory build()
}
class JsonFactory {
}
class ObjectMapper {
}
FeaturesConfig "1" <.. "*" ServerMainModule : reads
ServerMainModule ..> ObjectMapper : binds
ServerMainModule ..> JsonObjectMapperProvider : uses when jsonInternFieldNamesDisabled is false
ServerMainModule ..> PrestoJsonObjectMapperProvider : uses when jsonInternFieldNamesDisabled is true
PrestoJsonObjectMapperProvider --|> ObjectMapperProvider
JsonObjectMapperProvider --|> ObjectMapperProvider
PrestoJsonObjectMapperProvider ..> JsonFactoryBuilder : constructs
JsonFactoryBuilder ..> JsonFactory : builds
ObjectMapperProvider ..> ObjectMapper : provides
Flow diagram for json-intern-field-names-disabled configuration to runtime behaviorflowchart TD
cfg["Configuration property json-intern-field-names-disabled"]
fc["FeaturesConfig.jsonInternFieldNamesDisabled"]
smm["ServerMainModule checks isJsonInternFieldNamesDisabled"]
bindDisabled["Bind ObjectMapper to PrestoJsonObjectMapperProvider"]
bindEnabled["Bind ObjectMapper to JsonObjectMapperProvider"]
omDisabled["ObjectMapper created with INTERN_FIELD_NAMES disabled"]
omEnabled["ObjectMapper created with default INTERN_FIELD_NAMES"]
cfg --> fc
fc --> smm
smm -->|true| bindDisabled
smm -->|false| bindEnabled
bindDisabled --> omDisabled
bindEnabled --> omEnabled
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Consider adding a
@ConfigDescriptiontosetJsonInternFieldNamesDisabledto make the behavior and intended usage of the new configuration property clearer alongside the other feature flags. - Instead of introducing a separate
PrestoJsonObjectMapperProvider, consider extending/configuring the existingJsonObjectMapperProviderto take a flag or factory customization so that ObjectMapper construction remains centralized and easier to evolve.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider adding a `@ConfigDescription` to `setJsonInternFieldNamesDisabled` to make the behavior and intended usage of the new configuration property clearer alongside the other feature flags.
- Instead of introducing a separate `PrestoJsonObjectMapperProvider`, consider extending/configuring the existing `JsonObjectMapperProvider` to take a flag or factory customization so that ObjectMapper construction remains centralized and easier to evolve.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
8c00e2b to
ee374df
Compare
ee374df to
3b5d2c3
Compare
presto-main/src/main/java/com/facebook/presto/server/ServerMainModule.java
Outdated
Show resolved
Hide resolved
3b5d2c3 to
484a8e4
Compare
|
Do we need documentation anywhere for this config? |
630ce4f to
3223a32
Compare
3223a32 to
64a54d8
Compare
|
looks like the unit test failures are related to prestodb/airlift#126 in airlift. The error in unit tests indicates that the script is using a Python f-string (introduced in Python 3.6), but the interpreter running the script is an older version of Python (likely Python 2.x or <3.6). So we need to ensure that the Docker container is running Python 3.6 or newer. |
|
@NikhilCollooru I ran the failing tests locally and found they use The image docker run prestodb/centos7-oj8:11 python --version
Python 2.7.5Meanwhile, the latest Presto release (0.296) uses Python 3.9.25: docker run -it --entrypoint=/bin/bash prestodb/presto:0.296
> python --version
Python 3.9.25 |
|
@tdcmeehan do we have any image that uses python3 ? any suggestions on how to workaround this ? |
|
Can we just update our launcher script to use python3, instead of using the python alias? |
can you please point to the launcher script ...which can be modified to use python3 ? |
I’m afraid that |
|
@tdcmeehan any suggestions ? |
|
prestodb/airlift#144 |
64a54d8 to
29c3a4c
Compare
5f030fb to
551b3b5
Compare
56254e1 to
f86150d
Compare
f86150d to
600e6ac
Compare
## Description Update airlift version to include json serde contention fix. ## Motivation and Context We observed InternCache lock contention during high-concurrency JSON deserialization in remote task communication. The fix is in airlift. So update the version. ## Impact Improved performance of the coordinator at higher concurrency with lesser contention ## Test Plan Unit tests and verifier testing. ## Contributor checklist - [x] Please make sure your submission complies with our [contributing guide](https://github.com/prestodb/presto/blob/master/CONTRIBUTING.md), in particular [code style](https://github.com/prestodb/presto/blob/master/CONTRIBUTING.md#code-style) and [commit standards](https://github.com/prestodb/presto/blob/master/CONTRIBUTING.md#commit-standards). - [x] PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced. - [x] Documented new properties (with its default value), SQL syntax, functions, or other functionality. - [x] If release notes are required, they follow the [release notes guidelines](https://github.com/prestodb/presto/wiki/Release-Notes-Guidelines). - [x] Adequate tests were added if applicable. - [x] CI passed. - [x] If adding new dependencies, verified they have an [OpenSSF Scorecard](https://securityscorecards.dev/#the-checks) score of 5.0 or higher (or obtained explicit TSC approval for lower scores). ## Release Notes Please follow [release notes guidelines](https://github.com/prestodb/presto/wiki/Release-Notes-Guidelines) and fill in the release notes below. ``` == NO RELEASE NOTES == ```
## Description Update airlift version to include json serde contention fix. ## Motivation and Context We observed InternCache lock contention during high-concurrency JSON deserialization in remote task communication. The fix is in airlift. So update the version. ## Impact Improved performance of the coordinator at higher concurrency with lesser contention ## Test Plan Unit tests and verifier testing. ## Contributor checklist - [x] Please make sure your submission complies with our [contributing guide](https://github.com/prestodb/presto/blob/master/CONTRIBUTING.md), in particular [code style](https://github.com/prestodb/presto/blob/master/CONTRIBUTING.md#code-style) and [commit standards](https://github.com/prestodb/presto/blob/master/CONTRIBUTING.md#commit-standards). - [x] PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced. - [x] Documented new properties (with its default value), SQL syntax, functions, or other functionality. - [x] If release notes are required, they follow the [release notes guidelines](https://github.com/prestodb/presto/wiki/Release-Notes-Guidelines). - [x] Adequate tests were added if applicable. - [x] CI passed. - [x] If adding new dependencies, verified they have an [OpenSSF Scorecard](https://securityscorecards.dev/#the-checks) score of 5.0 or higher (or obtained explicit TSC approval for lower scores). ## Release Notes Please follow [release notes guidelines](https://github.com/prestodb/presto/wiki/Release-Notes-Guidelines) and fill in the release notes below. ``` == NO RELEASE NOTES == ```
Description
Update airlift version to include json serde contention fix.
Motivation and Context
We observed InternCache lock contention during high-concurrency JSON deserialization in remote task communication. The fix is in airlift. So update the version.
Impact
Improved performance of the coordinator at higher concurrency with lesser contention
Test Plan
Unit tests and verifier testing.
Contributor checklist
Release Notes
Please follow release notes guidelines and fill in the release notes below.