Copilot commented on code in PR #13879:
URL: https://github.com/apache/skywalking/pull/13879#discussion_r3285272002
##########
oap-server/server-storage-plugin/storage-banyandb-plugin/src/main/java/org/apache/skywalking/oap/server/storage/plugin/banyandb/BanyanDBIndexInstaller.java:
##########
@@ -806,21 +824,29 @@ private void checkMeasure(Model model, Measure measure,
BanyanDBClient client, S
opt.recordOutcome("measure",
hisMeasure.getMetadata().getName(),
StorageManipulationOpt.Outcome.SKIPPED_SHAPE_MISMATCH,
"backend shape differs from declared shape; use
/runtime/rule/addOrUpdate to reshape");
- return;
+ return false;
}
// banyanDB server can not delete or update Tags.
opt.recordModRevision(client.update(measure));
log.info("update Measure: {} from: {} to: {}",
hisMeasure.getMetadata().getName(), hisMeasure, measure);
}
}
+ return true;
}
/**
* Check if the stream exists and update (or record shape mismatch) per
mode.
* See {@link #checkMeasure} for the create-if-absent vs full-install
contract,
* including the {@link Model#isAllowBootReshape()} additive opt-in.
*/
- private void checkStream(Model model, Stream stream, BanyanDBClient
client, StorageManipulationOpt opt) throws BanyanDBException {
+ /**
+ * @return {@code true} when the live stream is now aligned with the
declared shape
Review Comment:
`checkStream(...)` also has two consecutive Javadoc blocks. This leaves a
stray `/** ... */` comment not attached to any declaration and duplicates the
docs. Combine them into a single Javadoc immediately above the method.
##########
oap-server/server-storage-plugin/storage-banyandb-plugin/src/main/java/org/apache/skywalking/oap/server/storage/plugin/banyandb/BanyanDBIndexInstaller.java:
##########
@@ -796,7 +814,7 @@ private void checkMeasure(Model model, Measure measure,
BanyanDBClient client, S
opt.recordOutcome("measure",
hisMeasure.getMetadata().getName(),
StorageManipulationOpt.Outcome.UPDATED,
"additive boot reshape: new tag / field added");
Review Comment:
The outcome message for the additive boot reshape only mentions adding new
tags/fields, but this PR also treats tag relocation (storage-only ↔ searchable)
as additive. Updating the message makes operator-facing outcomes less
misleading.
##########
oap-server/server-storage-plugin/storage-banyandb-plugin/src/main/java/org/apache/skywalking/oap/server/storage/plugin/banyandb/BanyanDBIndexInstaller.java:
##########
@@ -878,12 +905,42 @@ private boolean canBootReshape(Model model,
StorageManipulationOpt opt) {
}
/**
- * Purely-additive diff for a BanyanDB {@link Stream}: declared may add
tag families or
- * tags, but every tag-family / tag that already exists on the backend
must be present
- * with the same name and {@link BanyandbDatabase.TagType type}, the
{@link BanyandbDatabase.Entity entity}
- * column list must match exactly (reshape can't change shard / series-id
semantics),
- * and no tag may be dropped. Returns false for any non-additive
divergence so the caller
- * falls back to {@link
StorageManipulationOpt.Outcome#SKIPPED_SHAPE_MISMATCH}.
+ * Record a parallel {@link
StorageManipulationOpt.Outcome#SKIPPED_SHAPE_MISMATCH} for the
+ * index-rule + binding (+ TopN, for measures) resources of a stream /
measure / trace
Review Comment:
The Javadoc says this helper records outcomes for "index-rule + binding (+
TopN, for measures)", but `skipDependentReconcile` only records outcomes for
index rules/binding. Either record a TopN outcome too, or remove the TopN
mention to keep the comment accurate.
##########
oap-server/server-storage-plugin/storage-banyandb-plugin/src/main/java/org/apache/skywalking/oap/server/storage/plugin/banyandb/BanyanDBIndexInstaller.java:
##########
@@ -878,12 +905,42 @@ private boolean canBootReshape(Model model,
StorageManipulationOpt opt) {
}
/**
- * Purely-additive diff for a BanyanDB {@link Stream}: declared may add
tag families or
- * tags, but every tag-family / tag that already exists on the backend
must be present
- * with the same name and {@link BanyandbDatabase.TagType type}, the
{@link BanyandbDatabase.Entity entity}
- * column list must match exactly (reshape can't change shard / series-id
semantics),
- * and no tag may be dropped. Returns false for any non-additive
divergence so the caller
- * falls back to {@link
StorageManipulationOpt.Outcome#SKIPPED_SHAPE_MISMATCH}.
+ * Record a parallel {@link
StorageManipulationOpt.Outcome#SKIPPED_SHAPE_MISMATCH} for the
+ * index-rule + binding (+ TopN, for measures) resources of a stream /
measure / trace
+ * whose primary {@code check*} just skipped. Calling
+ * {@code checkIndexRules} / {@code checkIndexRuleBinding} unconditionally
after a primary
+ * skip would silently update the binding to reference the new declared
rule list while
+ * the underlying schema still carries the old shape — operators end up
with a binding
+ * pointing at tags / fields that don't agree with the live tag family
layout (e.g. a tag
+ * was dropped from the declared model but kept on the backend, the
binding loses its
+ * reference, and the orphan IndexRule becomes unqueryable).
+ *
+ * <p>Skipping the dependent reconcile keeps live state coherent: either
everything
+ * matches the declared shape, or nothing on this resource is touched
until the operator
+ * drops + recreates.
+ */
+ private void skipDependentReconcile(StorageManipulationOpt opt, String
resourceType, String resourceName) {
+ log.warn("BanyanDB {} {} shape mismatch — skipping dependent IndexRule
/ IndexRuleBinding "
+ + "reconciliation to avoid partial reshape (binding would
point at the new tag "
+ + "list while the live tag families still carry the old
shape).",
+ resourceType, resourceName);
+ opt.recordOutcome("indexRules", resourceName,
+ StorageManipulationOpt.Outcome.SKIPPED_SHAPE_MISMATCH,
+ resourceType + " shape mismatch; index-rule reconcile skipped");
Review Comment:
`StorageManipulationOpt.ResourceOutcome` documents resourceType labels like
`indexRule` / `indexRuleBinding`
(server-core/.../StorageManipulationOpt.java:455-457), and this class
consistently uses `indexRule` elsewhere. Recording `indexRules` here introduces
a new label that likely won't be recognized/filtered consistently in operator
output.
##########
oap-server/server-storage-plugin/storage-banyandb-plugin/src/main/java/org/apache/skywalking/oap/server/storage/plugin/banyandb/BanyanDBIndexInstaller.java:
##########
@@ -840,7 +866,7 @@ private void checkStream(Model model, Stream stream,
BanyanDBClient client, Stor
opt.recordOutcome("stream",
hisStream.getMetadata().getName(),
StorageManipulationOpt.Outcome.UPDATED,
"additive boot reshape: new tag added");
Review Comment:
Same as the measure path: this outcome message says "new tag added", but an
additive boot reshape may also be a tag relocation between families. Align the
message with the supported behavior so the recorded outcome is accurate.
##########
oap-server/server-storage-plugin/storage-banyandb-plugin/src/main/java/org/apache/skywalking/oap/server/storage/plugin/banyandb/BanyanDBIndexInstaller.java:
##########
@@ -776,7 +785,16 @@ private long defineIndexRuleBinding(List<IndexRule>
indexRules,
* {@link
org.apache.skywalking.oap.server.core.storage.model.ModelInstaller#whenCreating}
* so only one node races on the DDL.
*/
- private void checkMeasure(Model model, Measure measure, BanyanDBClient
client, StorageManipulationOpt opt) throws BanyanDBException {
+ /**
+ * @return {@code true} when the live measure is now aligned with the
declared shape
+ * (either it already matched, or the installer successfully
applied an update);
+ * {@code false} when the shape diverged and the installer recorded
Review Comment:
There are two consecutive Javadoc blocks immediately before
`checkMeasure(...)`. The first `/** ... */` is no longer attached to any symbol
and will be treated as a stray Javadoc comment, which can trip style checks and
makes the method docs harder to follow. Merge the two blocks into a single
Javadoc on the method.
##########
oap-server/server-storage-plugin/storage-banyandb-plugin/src/test/java/org/apache/skywalking/oap/server/storage/plugin/banyandb/BanyanDBIT.java:
##########
@@ -608,6 +608,143 @@ public void
testStreamNonAdditiveBootReshape_optInStillSkips() throws Exception
"expected SKIPPED_SHAPE_MISMATCH for non-additive (type-change)
diff even with opt-in, got " + bootOpt.getOutcomes());
}
+ /**
+ * Toggling {@code storageOnly} on an existing {@code @Column} moves the
tag from
+ * {@code storage-only} → {@code searchable} (or vice versa). Although the
live tag
+ * family no longer contains the tag at its old position, the tag identity
+ type are
+ * preserved, so {@link BanyanDBIndexInstaller#isPurelyAdditiveStream} (via
+ * {@code isPurelyAdditiveTagFamilies}) should accept the relocation when
+ * {@code allowBootReshape = true} and the OAP is in the init / standalone
path. The
+ * dependent IndexRule for the now-indexed tag should also be created.
+ */
+ @Test
+ public void testStreamStorageOnlyTogglePathBootReshape() throws Exception {
+ DownSamplingConfigService downSamplingConfigService = new
DownSamplingConfigService(Arrays.asList("minute"));
+ ModuleManager moduleManager = mock(ModuleManager.class);
+ ModuleProviderHolder moduleProviderHolder =
mock(ModuleProviderHolder.class);
+ ModuleServiceHolder moduleServiceHolder =
mock(ModuleServiceHolder.class);
+
when(moduleManager.find(CoreModule.NAME)).thenReturn(moduleProviderHolder);
+ when(moduleProviderHolder.provider()).thenReturn(moduleServiceHolder);
+
when(moduleServiceHolder.getService(DownSamplingConfigService.class)).thenReturn(downSamplingConfigService);
+
+ StorageModels models = new StorageModels();
+ Model baseModel = models.add(TestStreamStorageOnly.class,
DefaultScopeDefine.SERVICE,
+ new Storage("relocStream", true,
DownSampling.Second),
+
StorageManipulationOpt.withSchemaChange());
+ BanyanDBIndexInstaller installer = new BanyanDBIndexInstaller(client,
moduleManager, config);
+ installer.isExists(baseModel,
StorageManipulationOpt.withSchemaChange());
+ installer.createTable(baseModel);
+
+ String groupName = MetadataRegistry.convertGroupName(
+ config.getGlobal().getNamespace(),
BanyanDB.StreamGroup.RECORDS_LOG.getName());
+ BanyandbDatabase.Stream initial = client.client.findStream(groupName,
"relocStream");
+ // payload starts in storage-only family
+ assertTrue(initial.getTagFamiliesList().stream()
+ .filter(f -> "storage-only".equals(f.getName()))
+ .flatMap(f -> f.getTagsList().stream())
+ .anyMatch(t -> "payload".equals(t.getName())),
+ "expected payload tag in storage-only family initially, got " +
initial);
+
+ models.remove(TestStreamStorageOnly.class,
StorageManipulationOpt.withSchemaChange());
+ Model reshapedModel = models.add(TestStreamStorageOnlyOff.class,
DefaultScopeDefine.SERVICE,
+ new Storage("relocStream", true,
DownSampling.Second),
+
StorageManipulationOpt.withSchemaChange());
+ assertTrue(reshapedModel.isAllowBootReshape());
+
+ StorageManipulationOpt bootOpt =
StorageManipulationOpt.schemaCreateIfAbsent();
+ new BanyanDBIndexInstaller(client, moduleManager,
config).isExists(reshapedModel, bootOpt);
+
+ BanyandbDatabase.Stream reshaped = client.client.findStream(groupName,
"relocStream");
+ // payload is now in searchable family
+ assertTrue(reshaped.getTagFamiliesList().stream()
+ .filter(f -> "searchable".equals(f.getName()))
+ .flatMap(f -> f.getTagsList().stream())
+ .anyMatch(t -> "payload".equals(t.getName())),
+ "expected payload tag relocated to searchable family after
reshape, got " + reshaped);
+ assertFalse(reshaped.getTagFamiliesList().stream()
+ .filter(f -> "storage-only".equals(f.getName()))
+ .flatMap(f -> f.getTagsList().stream())
+ .anyMatch(t -> "payload".equals(t.getName())),
+ "expected payload tag no longer in storage-only family after
reshape, got " + reshaped);
+
+ boolean updatedRecorded = bootOpt.getOutcomes().stream()
+ .anyMatch(o -> "stream".equals(o.getResourceType())
+ && "relocStream".equals(o.getResourceName())
+ && o.getStatus() == StorageManipulationOpt.Outcome.UPDATED);
+ assertTrue(updatedRecorded, "expected UPDATED outcome for storageOnly
relocation, got " + bootOpt.getOutcomes());
Review Comment:
This test validates the tag family relocation, but it doesn't assert that
the dependent `IndexRule` and `IndexRuleBinding` were created/updated for the
newly-indexed `payload` tag. Adding assertions here would directly cover the
behavior this PR is enabling (relocation should allow index-rule reconciliation
to proceed).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
