1.18 ports9 #7281
1.18 ports9 #7281
Conversation
Scrool fix 1.18 backport
Backport of memory settings fixups from https://github.com/TouK/nussk…
📝 WalkthroughWalkthroughThe pull request introduces several modifications across different components of the Nussknacker application. The The OpenAPI specification has been expanded with new endpoints for scenario label management and updated existing endpoints with refined request and response schemas. The changelog is updated for version 1.18.1, detailing several bug fixes and enhancements. Modifications to the Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
utils/utils/src/main/scala/pl/touk/nussknacker/engine/util/json/ToJsonEncoder.scala (1)
89-92: Map encoding logic simplified but consider handling NU TypesThe map encoding logic has been simplified, which is good. However, there's a TODO comment about making the encoder aware of NU Types for special cases like multiset.
Would you like help implementing the NU Types-aware encoding mentioned in the TODO comment? This could improve the handling of special types like multiset.
scenario-compiler/src/test/scala/pl/touk/nussknacker/engine/spel/SpelExpressionSpec.scala (1)
2073-2091: Optimize resource usage in concurrency testThe test initializes a thread pool with 1000 threads and creates 10,000 tasks, which is resource-intensive and might not be necessary to reproduce the race condition. Consider reducing the number of threads and tasks or using more efficient concurrency testing strategies to achieve the same goals with fewer resources.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (10)
designer/restmodel/src/main/scala/pl/touk/nussknacker/restmodel/definition/package.scala(1 hunks)designer/server/src/main/scala/pl/touk/nussknacker/ui/definition/DefinitionsService.scala(1 hunks)docs-internal/api/nu-designer-openapi.yaml(1 hunks)docs/Changelog.md(1 hunks)examples/installation/docker-compose.yml(2 hunks)examples/installation/flink/flink-properties.yml(1 hunks)scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/spel/SpelExpression.scala(2 hunks)scenario-compiler/src/test/scala/pl/touk/nussknacker/engine/spel/SpelExpressionSpec.scala(3 hunks)utils/utils/src/main/scala/pl/touk/nussknacker/engine/util/json/ToJsonEncoder.scala(1 hunks)utils/utils/src/test/scala/pl/touk/nussknacker/engine/util/json/ToJsonEncoderSpec.scala(1 hunks)
🧰 Additional context used
📓 Learnings (2)
scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/spel/SpelExpression.scala (1)
Learnt from: arkadius
PR: TouK/nussknacker#7174
File: scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/spel/SpelExpression.scala:120-132
Timestamp: 2024-11-19T21:59:39.327Z
Learning: When handling re-parsing of expressions in the `renderExpression` method of `renderTemplateExpressionParts` in `SpelExpression.scala`, re-parsing is complex and expressions should be re-parsed once more without force compile.
scenario-compiler/src/test/scala/pl/touk/nussknacker/engine/spel/SpelExpressionSpec.scala (1)
Learnt from: arkadius
PR: TouK/nussknacker#7174
File: scenario-compiler/src/test/scala/pl/touk/nussknacker/engine/spel/SpelExpressionSpec.scala:84-84
Timestamp: 2024-11-19T22:01:38.151Z
Learning: In test classes such as `SpelExpressionSpec.scala` within the `scenario-compiler` module, binary compatibility issues are not a concern because they are used only internally. Therefore, changes to method signatures in these test classes are acceptable.
🔇 Additional comments (10)
designer/restmodel/src/main/scala/pl/touk/nussknacker/restmodel/definition/package.scala (1)
53-56: Well-documented change for backward compatibility!
The comments clearly explain the purpose of making requiredParam optional:
- Used by external project
- Introduced in 1.18 version
- Option type enables decoder backward compatibility
- Can be cleaned up in future releases
designer/server/src/main/scala/pl/touk/nussknacker/ui/definition/DefinitionsService.scala (1)
175-175: LGTM - Consistent with the core change!
The service implementation correctly wraps the existing boolean logic in Some() to match the new Option[Boolean] type while maintaining the same behavior.
docs-internal/api/nu-designer-openapi.yaml (1)
7029-7031: LGTM - API spec correctly updated!
The OpenAPI specification has been properly updated to reflect the nullable requiredParam field, maintaining consistency with the code changes.
utils/utils/src/test/scala/pl/touk/nussknacker/engine/util/json/ToJsonEncoderSpec.scala (1)
104-118: Well-structured test case for map order preservation
The test case effectively verifies the order preservation of map keys using ListMap. Good coverage of different data types and clear explanation of the string comparison approach.
Consider adding a few more test scenarios:
- Empty map
- Map with null values
- Nested maps
it("should convert map to json and keep order of keys") {
val map = ListMap(
"intNumber" -> 42,
"floatNumber" -> 42.42,
"someTimestamp" -> 1496930555793L,
"someString" -> "hello",
- "booleanValue" -> true
+ "booleanValue" -> true,
+ "nullValue" -> null,
+ "nestedMap" -> ListMap("key1" -> "value1")
)
val expectedJson =
- """{"intNumber":42,"floatNumber":42.42,"someTimestamp":1496930555793,"someString":"hello","booleanValue":true}"""
+ """{"intNumber":42,"floatNumber":42.42,"someTimestamp":1496930555793,"someString":"hello","booleanValue":true,"nullValue":null,"nestedMap":{"key1":"value1"}}"""
// We compare string because we want to check the order
encoder.encode(map).noSpaces shouldBe expectedJson
}
+it("should handle empty map") {
+ encoder.encode(ListMap.empty[String, Any]).noSpaces shouldBe "{}"
+}examples/installation/docker-compose.yml (2)
244-244: Memory limit increase for flink-taskmanager looks good.
The increase from 1024M to 1500M aligns with the memory-related properties in flink/flink-properties.yml mentioned in the changelog.
43-43: Memory configuration changes for designer service need verification.
The JVM memory settings have been significantly reduced and constrained:
- Max heap reduced from 1024M to 400M
- Added explicit limits for metaspace (300M) and direct memory (100M)
✅ Verification successful
Based on the analysis of the codebase and configuration files, I can now provide a definitive response about the memory configuration changes:
Memory configuration for designer service appears safe and well-balanced
The reduced memory limits are part of a well-structured container resource allocation strategy where:
- The 400MB heap is sufficient as the designer service primarily handles UI and configuration operations
- The 300MB metaspace allocation aligns with typical JVM needs for class metadata
- The 100MB direct memory limit is adequate for non-heap operations
- The configuration is consistent with other services' memory allocations in the stack (e.g., Kafka: 256MB, Schema Registry: 512MB)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify if there are any memory-related issues reported in designer logs
docker-compose logs designer | grep -iE "OutOfMemoryError|GC|heap|memory"
Length of output: 180
Script:
#!/bin/bash
# Let's analyze the designer service configuration and resource usage patterns
# 1. Check for any documentation about memory requirements
fd README -t f -X cat {}
# 2. Look for any memory-related configuration or documentation
rg -i "memory|heap|jvm" -A 3 -B 3
# 3. Search for designer service configuration patterns
rg -i "designer.*memory|designer.*heap|designer.*jvm" -A 3 -B 3
# 4. Look for any performance or memory-related tests
fd test -t f -X rg -i "memory|performance|heap" {}
Length of output: 119589
scenario-compiler/src/main/scala/pl/touk/nussknacker/engine/spel/SpelExpression.scala (2)
35-35: Added thread-safe flag for tracking first interpretation.
Good addition of AtomicBoolean for thread-safe tracking of the first interpretation state.
Also applies to: 56-57
60-73: Well-implemented synchronization for first interpretation.
The synchronization block correctly handles the race condition in Spring's SpelExpression class. The implementation:
- Only synchronizes during first interpretation
- Sets the flag after successful interpretation
- Returns to normal execution for subsequent calls
docs/Changelog.md (1)
43-51: Changelog entries accurately reflect the implemented changes.
The entries properly document:
- Various bug fixes including the SpEL expression race condition fix
- Memory and container configuration changes
scenario-compiler/src/test/scala/pl/touk/nussknacker/engine/spel/SpelExpressionSpec.scala (1)
2099-2104: 🛠️ Refactor suggestion
Avoid relying on exception messages for control flow
At lines 2099 to 2104, the code pattern-matches on the exception message string to determine failure cases. This approach is brittle since exception messages can change, and it might not reliably capture the intended exceptions. Consider catching specific exception types or introducing custom exceptions to handle this scenario more robustly.
As a follow-up, you might revise the pattern matching to use exception types:
- case Failure(e: IllegalStateException) if e.getMessage == "Failed to instantiate CompiledExpression" =>
+ case Failure(e: SpecificExceptionType) =>Replace SpecificExceptionType with the appropriate exception class that accurately represents the failure condition.
Likely invalid or redundant comment.
| # Nu requires a little bit more metaspace than Flink default allocate based on process size | ||
| taskmanager.memory.process.size: 1500m | ||
| taskmanager.memory.jvm-metaspace.size: 400m |
There was a problem hiding this comment.
💡 Codebase verification
Memory configuration needs adjustment based on test configuration
The metaspace setting of 400m in the production configuration is actually lower than the 512m used in test configuration. Since the comment indicates Nu requires more metaspace than Flink's default, and test configuration uses 512m, consider increasing the metaspace size to at least match the test setting.
Key findings:
- Test configuration uses
taskmanager.memory.jvm-metaspace.size: 512m - Production setting of 400m might be insufficient
- Total process size of 1500m aligns with docker-compose memory limit
🔗 Analysis chain
Memory configuration looks reasonable but verify against Flink's guidelines
The added memory settings with explanation comment are good. However, let's verify these values:
taskmanager.memory.process.size: 1500maligns with the docker-compose memory limittaskmanager.memory.jvm-metaspace.size: 400mis quite large for metaspace
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if these memory settings are used consistently across the codebase
rg -l "taskmanager\.memory\.(process\.size|jvm-metaspace\.size)"
# Look for any other memory-related configurations that might need adjustment
rg "memory" examples/installation/
Length of output: 1325
Script:
#!/bin/bash
# Check the test configuration to compare memory settings
cat engine/flink/test-utils/src/main/resources/docker/conf.yml
# Look for any documentation about memory settings
fd README.md examples/installation/
Length of output: 360
| // This test is ignored as it was indeterministic and ugly, but it was used to verify race condition problems on | ||
| // ParsedSpelExpression.getValue. Without the synchronized block inside its method the test would fail the majority of times | ||
| ignore( | ||
| "should not throw 'Failed to instantiate CompiledExpression' when getValue is called on ParsedSpelExpression by multiple threads" | ||
| ) { | ||
| val spelExpression = | ||
| parse[LocalDateTime]("T(java.time.LocalDateTime).now().minusDays(14)", ctx).validValue.expression | ||
| .asInstanceOf[SpelExpression] | ||
|
|
||
| val threadPool = Executors.newFixedThreadPool(1000) | ||
| implicit val customExecutionContext: ExecutionContext = ExecutionContext.fromExecutor(threadPool) | ||
|
|
||
| // A promise to signal when an exception occurs | ||
| val failurePromise = Promise[Unit]() | ||
|
|
||
| val tasks = (1 to 10000).map { _ => | ||
| Future { | ||
| try { | ||
| Thread.sleep(100) | ||
| // evaluate calls getValue on problematic SpelExpression object | ||
| spelExpression.evaluate[LocalDateTime](Context("fooId"), Map.empty) | ||
| } catch { | ||
| // The real problematic exception is wrapped in SpelExpressionEvaluationException by evaluate method | ||
| case e: SpelExpressionEvaluationException => | ||
| failurePromise.tryFailure(e.cause) | ||
| } | ||
| } | ||
| } | ||
| val firstFailureOrCompletion = Future.firstCompletedOf(Seq(Future.sequence(tasks), failurePromise.future)) | ||
|
|
||
| firstFailureOrCompletion.onComplete { | ||
| case Success(_) => | ||
| println("All tasks completed successfully.") | ||
| threadPool.shutdown() | ||
| case Failure(e: IllegalStateException) if e.getMessage == "Failed to instantiate CompiledExpression" => | ||
| fail("Exception occurred due to race condition.", e) | ||
| threadPool.shutdown() | ||
| case Failure(e) => | ||
| fail("Unknown exception occurred", e) | ||
| threadPool.shutdown() | ||
| } | ||
| Await.result(firstFailureOrCompletion, 15.seconds) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Rework or remove the ignored test to maintain code quality
The test from lines 2064 to 2106 is currently ignored and noted as indeterministic and "ugly". Ignored tests can clutter the codebase and impede maintenance. If the race condition it addresses is still relevant, consider refactoring the test to be deterministic and reliable. If it's obsolete, removing it would enhance code cleanliness.
| Thread.sleep(100) | ||
| // evaluate calls getValue on problematic SpelExpression object | ||
| spelExpression.evaluate[LocalDateTime](Context("fooId"), Map.empty) | ||
| } catch { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid blocking calls within Futures
At lines 2082 to 2085, Thread.sleep(100) is used inside a Future, which blocks the thread and can degrade performance in asynchronous code. Consider refactoring to use non-blocking constructs or appropriate synchronization mechanisms to simulate delays without blocking threads.
For example, you can use scala.concurrent.Promise and scala.concurrent.duration._ to achieve a delay without blocking:
- Thread.sleep(100)
+ Await.ready(Promise().future, 100.milliseconds)This approach leverages non-blocking constructs to introduce delays in asynchronous code.
Committable suggestion skipped: line range outside the PR's diff.
Describe your changes
Checklist before merge
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Tests
Chores