temporarily putting PPL Alerting serde behind 3.6 check

[toepkerd]

Temporarily putting PPL Alerting fields under 3.6 until Alerting version is bumped to 3.7.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 912bd80)

Here are some key observations to aid the review process:

🧪 No relevant tests

🔒 No security concerns identified

📝 TODO sections src/main/kotlin/org/opensearch/commons/alerting/model/Alert.kt [420]: Change to 3.7 when alerting version bump happens src/main/kotlin/org/opensearch/commons/alerting/model/Alert.kt [426]: Change to 3.7 when alerting version bump happens src/main/kotlin/org/opensearch/commons/alerting/model/Alert.kt [475]: Change to 3.7 when alerting version bump happens src/main/kotlin/org/opensearch/commons/alerting/model/MonitorRunResult.kt [132]: Change to 3.7 when alerting version bump happens src/main/kotlin/org/opensearch/commons/alerting/model/MonitorRunResult.kt [153]: Change to 3.7 when alerting version bump happens

🔀 No multiple PR themes

⚡ Recommended focus areas for review Version Mismatch Risk The version check was changed from V_3_7_0 to V_3_6_0 as a temporary measure. If nodes running 3.6 and 3.7 coexist in a cluster, the serialization behavior may differ from what 3.7 nodes expect, potentially causing deserialization errors or data loss during rolling upgrades. The TODO comments acknowledge this, but the risk of forgetting to revert before 3.7 release is high. query = if (sin.version.onOrAfter(Version.V_3_6_0)) { sin.readOptionalString() } else { null }, // TODO: change to 3.7 when alerting version bump happens queryResults = if (sin.version.onOrAfter(Version.V_3_6_0)) { sin.readList { input -> suppressWarning(input.readMap()) } } else { listOf() }, Redundant Version Checks In readFrom, there are separate version checks for reading pplCount and then for iterating pplList, but if pplCount is 0 (the fallback), the loop body would never execute anyway. The extra version check for the loop is redundant and adds confusion. Additionally, pplNumResults has its own separate version check, making the code harder to maintain and increasing the chance of inconsistency when updating the version back to 3.7. val pplCount = if (sin.version.onOrAfter(Version.V_3_6_0)) { sin.readVInt() } else { 0 } val pplList = mutableListOf<Map<String, Any?>>() // TODO: change to 3.7 when alerting version bump happens if (sin.version.onOrAfter(Version.V_3_6_0)) { for (i in 0 until pplCount) { pplList.add(suppressWarning(sin.readMap())) // pplResults } } // TODO: change to 3.7 when alerting version bump happens val pplNumResults = if (sin.version.onOrAfter(Version.V_3_6_0)) { sin.readOptionalLong() } else { null }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 912bd80
Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Consolidate related version-guarded read operations The version check for pplCount and the subsequent loop reading pplList use separate onOrAfter calls, but they must always be evaluated together. If the version check logic ever diverges (e.g., during the TODO bump to 3.7), one could be updated without the other, causing a deserialization mismatch. Consider consolidating the read logic into a single version-guarded block to ensure atomicity and reduce the risk of inconsistency. src/main/kotlin/org/opensearch/commons/alerting/model/MonitorRunResult.kt [154-171] -val pplCount = if (sin.version.onOrAfter(Version.V_3_6_0)) { - sin.readVInt() - } else { - 0 - } - val pplList = mutableListOf<Map<String, Any?>>() +val pplList = mutableListOf<Map<String, Any?>>() + var pplCount = 0 // TODO: change to 3.7 when alerting version bump happens if (sin.version.onOrAfter(Version.V_3_6_0)) { + pplCount = sin.readVInt() for (i in 0 until pplCount) { pplList.add(suppressWarning(sin.readMap())) // pplResults } } // TODO: change to 3.7 when alerting version bump happens val pplNumResults = if (sin.version.onOrAfter(Version.V_3_6_0)) { sin.readOptionalLong() } else { null } Suggestion importance[1-10]: 5 __ Why: The suggestion correctly identifies that pplCount and the pplList loop should be in a single version-guarded block to avoid potential deserialization mismatches. However, pplNumResults still has a separate version check, so the consolidation is only partial. The improvement is valid but of moderate impact.	Low

---

Previous suggestions

Suggestions up to commit 912bd80

Category	Suggestion	Impact
Possible issue	Consolidate related version-gated reads into single block The version check for pplCount and the subsequent loop reading pplList use separate onOrAfter calls, but they must always be evaluated together since pplCount is only written when the version check passes. If the version check result somehow differs between the two calls (e.g., due to a bug or future refactor), pplCount would be 0 but the loop would still attempt to read, or vice versa. Consolidate into a single version check block to ensure consistency and avoid potential deserialization mismatches. src/main/kotlin/org/opensearch/commons/alerting/model/MonitorRunResult.kt [154-171] -val pplCount = if (sin.version.onOrAfter(Version.V_3_6_0)) { - sin.readVInt() - } else { - 0 - } - val pplList = mutableListOf<Map<String, Any?>>() +val pplList = mutableListOf<Map<String, Any?>>() // TODO: change to 3.7 when alerting version bump happens + val pplNumResults: Long? if (sin.version.onOrAfter(Version.V_3_6_0)) { + val pplCount = sin.readVInt() for (i in 0 until pplCount) { pplList.add(suppressWarning(sin.readMap())) // pplResults } - } - // TODO: change to 3.7 when alerting version bump happens - val pplNumResults = if (sin.version.onOrAfter(Version.V_3_6_0)) { - sin.readOptionalLong() + pplNumResults = sin.readOptionalLong() } else { - null + pplNumResults = null } Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies that having separate onOrAfter version checks for pplCount, the loop, and pplNumResults is redundant and could be fragile. Consolidating into a single block improves readability and ensures consistent deserialization behavior, though in practice the version check result won't differ between calls on the same sin object.	Low

[github-actions]

Persistent review updated to latest commit 912bd80