Conversation
github-actions
bot
added
backend
safe to test
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
openmetadata-service/src/main/java/org/openmetadata/service/apps/logging/RunLogBuffer.java
Outdated
Show resolved
Hide resolved
...ice/src/main/java/org/openmetadata/service/apps/bundles/searchIndex/SearchIndexExecutor.java
Outdated
Show resolved
Hide resolved
openmetadata-service/src/main/java/org/openmetadata/service/apps/logging/AppRunLogAppender.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
This PR introduces infrastructure to capture and persist per-application-run logs (notably for reindex/SearchIndex runs) and improves SearchIndex stats accuracy/refresh behavior, backed by new unit tests.
Changes:
- Add a Logback
Appender+ buffered file writer to capture app-run logs intologs/app-runs/{appName}/{timestamp}-{serverId}.log. - Wire log-capture lifecycle into Quartz job execution via
OmAppJobListener(start/stop capture, MDC setup/cleanup, thread-prefix capture for reindex workers). - Improve SearchIndex stats consistency by adjusting totals when processed counts exceed initial totals and periodically syncing sink stats; add tests for these cases.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| openmetadata-service/src/main/java/org/openmetadata/service/apps/scheduler/OmAppJobListener.java | Starts/stops app-run log capture around Quartz job execution and manages MDC/thread-prefix capture. |
| openmetadata-service/src/main/java/org/openmetadata/service/apps/logging/AppRunLogAppender.java | New Logback appender that routes matching events to per-run buffers/files and performs retention cleanup. |
| openmetadata-service/src/main/java/org/openmetadata/service/apps/logging/RunLogBuffer.java | New buffered writer with periodic flushing and max-line cap for app-run log persistence. |
| openmetadata-service/src/main/java/org/openmetadata/service/apps/bundles/searchIndex/SearchIndexExecutor.java | Periodic sink stat sync and total-records adjustments to keep stats internally consistent. |
| openmetadata-service/src/test/java/org/openmetadata/service/apps/logging/AppRunLogAppenderTest.java | New tests covering capture behavior, cleanup, listing timestamps, and concurrency safety of app-run logging. |
| openmetadata-service/src/test/java/org/openmetadata/service/apps/logging/RunLogBufferTest.java | New tests for buffer append/flush/close behavior and max-line handling. |
| openmetadata-service/src/test/java/org/openmetadata/service/apps/bundles/searchIndex/SearchIndexStatsTest.java | New tests validating adjusted totals and reader/job totals consistency. |
openmetadata-service/src/main/java/org/openmetadata/service/apps/logging/AppRunLogAppender.java
Show resolved
Hide resolved
| static void resetForTest() { | ||
| registered = false; | ||
| threadPrefixBindings.clear(); | ||
| } |
There was a problem hiding this comment.
resetForTest() only clears registered and threadPrefixBindings but leaves other static state mutated by tests (e.g., logDirectory, maxRunsPerApp, maxLinesPerRun) and does not remove the appender from the root logger if it was registered. This can leak state across test classes within the same JVM; consider resetting static defaults and detaching/stopping the root appender in resetForTest().
...metadata-service/src/main/java/org/openmetadata/service/apps/scheduler/OmAppJobListener.java
Show resolved
Hide resolved
| @AfterEach | ||
| void tearDown() { | ||
| AppRunLogAppender.getActiveBuffers().clear(); | ||
| AppRunLogAppender.resetForTest(); |
There was a problem hiding this comment.
setUp() mutates static state via AppRunLogAppender.setLogDirectoryForTest(...), but tearDown() does not restore it (and resetForTest() currently doesn’t reset logDirectory). This can leak configuration into other test classes in the same JVM. Restore the defaults in tearDown() (or enhance resetForTest() to reset logDirectory/limits and detach any root-logger appender registered by startCapture).
| AppRunLogAppender.resetForTest(); | |
| AppRunLogAppender.resetForTest(); | |
| AppRunLogAppender.setLogDirectoryForTest(null); |
| String batchText = String.join("\n", batch); | ||
| writeToFile(batchText); | ||
| AppRunLogMetrics.recordFlush(appName, batch.size()); | ||
| } |
There was a problem hiding this comment.
flush() calls AppRunLogMetrics.recordFlush(...), but there is no AppRunLogMetrics class in the codebase (repo-wide search only finds these references). This will fail compilation; either add the missing metrics implementation or remove/replace these calls with an existing metrics facility.
| void startFlusher() { | ||
| try { | ||
| Files.createDirectories(logFile.getParent()); | ||
| writer = | ||
| Files.newBufferedWriter( | ||
| logFile, | ||
| StandardCharsets.UTF_8, | ||
| StandardOpenOption.CREATE, | ||
| StandardOpenOption.APPEND); | ||
| } catch (IOException e) { | ||
| LOG.error("Failed to open log file {}: {}", logFile, e.getMessage()); | ||
| return; | ||
| } |
There was a problem hiding this comment.
If startFlusher() fails to open the log file (IOException), it logs and returns, but the buffer remains usable and will keep accumulating pending lines (up to maxLines) without ever flushing. Consider returning a success/failure signal (or throwing) so the caller can stop capture/remove the buffer, or mark the buffer as closed/disabled on failure.
| public static RunLogBuffer startCapture( | ||
| String appRunId, String appId, String appName, String serverId, String... threadPrefixes) { | ||
| ensureRegistered(); | ||
| cleanupOldRuns(appName); | ||
| Path logFile = resolveLogFile(appName, Long.parseLong(appRunId), serverId); | ||
| RunLogBuffer buffer = | ||
| new RunLogBuffer( | ||
| appId, appName, serverId, Long.parseLong(appRunId), maxLinesPerRun, logFile); | ||
| activeBuffers.put(bufferKey(appName, appRunId), buffer); | ||
|
|
||
| for (String prefix : threadPrefixes) { | ||
| threadPrefixBindings.add(new ThreadPrefixBinding(prefix, buffer)); | ||
| } | ||
|
|
||
| buffer.startFlusher(); | ||
| AppRunLogMetrics.recordRunStarted(appName, serverId); | ||
| return buffer; | ||
| } | ||
|
|
||
| public static void stopCapture(String appName, String appRunId) { | ||
| RunLogBuffer buffer = activeBuffers.remove(bufferKey(appName, appRunId)); | ||
| if (buffer != null) { | ||
| threadPrefixBindings.removeIf(b -> b.buffer == buffer); | ||
| long durationMs = System.currentTimeMillis() - buffer.getRunTimestamp(); | ||
| AppRunLogMetrics.recordRunCompleted(appName, buffer.getServerId(), durationMs); | ||
| AppRunLogMetrics.recordLinesCapture(appName, buffer.getTotalLineCount()); | ||
| buffer.close(); | ||
| } | ||
| } | ||
|
|
||
| public static RunLogBuffer getBuffer(String appName, String runTimestamp) { | ||
| return activeBuffers.get(bufferKey(appName, runTimestamp)); | ||
| } | ||
|
|
||
| static void cleanupOldRuns(String appName) { | ||
| List<Long> timestamps = listRunTimestamps(appName); | ||
| if (timestamps.size() <= maxRunsPerApp) { | ||
| return; | ||
| } | ||
| List<Long> toDelete = timestamps.subList(maxRunsPerApp, timestamps.size()); | ||
| Path appDir = resolveAppDir(appName); | ||
| for (long ts : toDelete) { | ||
| try (DirectoryStream<Path> stream = Files.newDirectoryStream(appDir, ts + "-*.log")) { | ||
| for (Path entry : stream) { | ||
| Files.deleteIfExists(entry); | ||
| } | ||
| } catch (IOException e) { | ||
| // best-effort cleanup | ||
| } | ||
| } | ||
| AppRunLogMetrics.recordCleanup(appName, toDelete.size()); | ||
| } |
There was a problem hiding this comment.
startCapture/stopCapture/cleanupOldRuns invoke AppRunLogMetrics.*, but there is no AppRunLogMetrics class in the repository (only these references). This makes the module uncompilable; add the missing metrics class or remove/replace these calls.
| buffer.startFlusher(); | ||
| AppRunLogMetrics.recordRunStarted(appName, serverId); | ||
| return buffer; |
There was a problem hiding this comment.
AppRunLogAppender references AppRunLogMetrics (e.g., recordRunStarted) but there is no AppRunLogMetrics class in the codebase, so this will not compile. Either add the missing metrics implementation (and wiring) or remove these calls / replace with the existing metrics facility used elsewhere in the service.
| * <p>Configured via the {@code logging:} section in {@code openmetadata.yaml}. Uses two-tier | ||
| * matching: MDC for the scheduler thread, thread name prefixes for worker threads. |
There was a problem hiding this comment.
The class-level Javadoc says this appender is configured via openmetadata.yaml, but the implementation self-registers programmatically via ensureRegistered() and does not read Dropwizard logging config. Please update the Javadoc to reflect the actual configuration/registration mechanism (and how operators can disable/enable it).
| * <p>Configured via the {@code logging:} section in {@code openmetadata.yaml}. Uses two-tier | |
| * matching: MDC for the scheduler thread, thread name prefixes for worker threads. | |
| * <p>This appender is registered programmatically and is not wired through Dropwizard's | |
| * {@code logging:} section in {@code openmetadata.yaml}. Call {@link #ensureRegistered()} once | |
| * during application startup to attach it to the appropriate {@link LoggerContext}. Operators | |
| * who do not want per-run app logs can avoid invoking {@code ensureRegistered()} (or remove this | |
| * appender from the {@link LoggerContext} at runtime). |
| void flush() { | ||
| List<String> batch = drainPending(); | ||
| if (batch.isEmpty()) { | ||
| return; | ||
| } | ||
| String batchText = String.join("\n", batch); | ||
| writeToFile(batchText); | ||
| } |
There was a problem hiding this comment.
flush() drains pending even when the file writer is not available (e.g., if startFlusher() failed to open the log file or was never called). Since writeToFile() returns early when writer == null, this causes buffered log lines to be dropped silently. Consider short-circuiting flush() when writer is null (or lazily opening/retrying the writer) so pending lines are preserved until they can be written.
| if (readerStats != null && totalRecords > readerStats.getTotalRecords()) { | ||
| readerStats.setTotalRecords(totalRecords); |
There was a problem hiding this comment.
readerStats.getTotalRecords() can be null (e.g., if reader stats were created elsewhere without initializing totalRecords). The comparison totalRecords > readerStats.getTotalRecords() will then throw an NPE. Please null-guard (treat null as 0) before comparing/updating the reader total.
| if (readerStats != null && totalRecords > readerStats.getTotalRecords()) { | |
| readerStats.setTotalRecords(totalRecords); | |
| if (readerStats != null) { | |
| Integer readerTotalRecords = readerStats.getTotalRecords(); | |
| int safeReaderTotalRecords = readerTotalRecords != null ? readerTotalRecords : 0; | |
| if (totalRecords > safeReaderTotalRecords) { | |
| readerStats.setTotalRecords(totalRecords); | |
| } |
| import java.lang.reflect.Field; | ||
| import java.util.List; | ||
| import java.util.UUID; | ||
| import org.junit.jupiter.api.BeforeEach; | ||
| import org.junit.jupiter.api.Test; | ||
| import org.junit.jupiter.api.extension.ExtendWith; | ||
| import org.mockito.Mock; | ||
| import org.mockito.MockedStatic; | ||
| import org.mockito.junit.jupiter.MockitoExtension; | ||
| import org.mockito.junit.jupiter.MockitoSettings; | ||
| import org.mockito.quality.Strictness; | ||
| import org.openmetadata.schema.entity.app.App; | ||
| import org.openmetadata.service.Entity; | ||
| import org.openmetadata.service.jdbi3.AppRepository; | ||
| import org.openmetadata.service.jdbi3.CollectionDAO; | ||
| import sun.misc.Unsafe; |
There was a problem hiding this comment.
This test uses sun.misc.Unsafe.allocateInstance() to instantiate AppResource. Accessing JDK internals via reflection is brittle on Java 21+ (module encapsulation) and can fail in CI without explicit --add-opens. Prefer constructing AppResource normally (or via a test-friendly constructor/factory) and injecting dependencies via setters/constructors, or use a Mockito spy/subclass to override the repository as needed.
| package org.openmetadata.service.apps.scheduler; | ||
|
|
||
| import static org.junit.jupiter.api.Assertions.assertFalse; |
There was a problem hiding this comment.
This file is missing the standard Apache 2.0 license header block before the package declaration. Please add the license header at the top of the file.
| import java.lang.reflect.Field; | ||
| import java.lang.reflect.Method; | ||
| import org.junit.jupiter.api.BeforeEach; | ||
| import org.junit.jupiter.api.Test; | ||
| import org.junit.jupiter.api.extension.ExtendWith; | ||
| import org.mockito.Mock; | ||
| import org.mockito.MockedStatic; | ||
| import org.mockito.junit.jupiter.MockitoExtension; | ||
| import org.mockito.junit.jupiter.MockitoSettings; | ||
| import org.mockito.quality.Strictness; | ||
| import org.openmetadata.schema.entity.app.App; | ||
| import org.openmetadata.service.Entity; | ||
| import org.openmetadata.service.apps.AbstractNativeApplication; | ||
| import org.openmetadata.service.apps.ApplicationHandler; | ||
| import org.openmetadata.service.jdbi3.CollectionDAO; | ||
| import org.openmetadata.service.search.SearchRepository; | ||
| import org.quartz.Scheduler; | ||
| import sun.misc.Unsafe; | ||
|
|
There was a problem hiding this comment.
This test uses sun.misc.Unsafe.allocateInstance() to instantiate AppScheduler. This is fragile on Java 21+ and may fail under module encapsulation. Prefer using a real constructor (or a package-private test constructor), or refactor AppScheduler to allow injecting the Scheduler dependency for tests without relying on Unsafe.
| package org.openmetadata.service.apps; | ||
|
|
||
| import static org.junit.jupiter.api.Assertions.assertFalse; |
There was a problem hiding this comment.
This file is missing the standard Apache 2.0 license header block before the package declaration. Please add the license header at the top of the file.
| const VECTOR_INDEXABLE_ENTITIES = new Set([ | ||
| 'table', | ||
| 'glossary', | ||
| 'glossaryterm', | ||
| 'chart', | ||
| 'dashboard', | ||
| 'dashboarddatamodel', | ||
| 'database', | ||
| 'databaseschema', | ||
| 'dataproduct', | ||
| 'pipeline', | ||
| 'mlmodel', | ||
| 'metric', | ||
| 'apiendpoint', | ||
| 'apicollection', | ||
| 'page', | ||
| 'storedprocedure', | ||
| 'searchindex', | ||
| 'topic', | ||
| ]); |
There was a problem hiding this comment.
The new constant VECTOR_INDEXABLE_ENTITIES hardcodes the list of entity types that support vector embeddings. This is likely to drift from backend capabilities over time; consider deriving vector-indexability from the stats payload itself (e.g., presence of vectorSuccessRecords/vectorFailedRecords) or from a single shared source of truth instead of duplicating the list in UI utils.
| package org.openmetadata.service.resources.apps; | ||
|
|
||
| import static org.junit.jupiter.api.Assertions.assertEquals; |
There was a problem hiding this comment.
This file is missing the standard Apache 2.0 license header block before the package declaration (consistent across other Java sources/tests in this repo). Please add the license header at the top of the file.
| "retry-count": "Contagem de tentativas", | ||
| "return": "بازگشت", |
There was a problem hiding this comment.
This locale file is predominantly Persian, but the newly added retry-count translation here is Portuguese. Please translate it to Persian to avoid mixed-language UI.
| "no-retry-queue-records": "Nenhum registro de tentativa de indexação ao vivo encontrado. Todas as entidades foram indexadas com sucesso.", | ||
| "no-roles-assigned": "هیچ نقشی اختصاص داده نشده است.", |
There was a problem hiding this comment.
This locale file is predominantly Persian, but the newly added no-retry-queue-records message here is Portuguese. Please translate it to Persian so users don’t see mixed-language messages.
| package org.openmetadata.service.apps.scheduler; | ||
|
|
There was a problem hiding this comment.
New Java test files are expected to include the Apache 2.0 license header before the package declaration (see e.g. openmetadata-service/src/test/java/org/openmetadata/service/jdbi3/BulkExecutorTest.java:1-13). Please add the standard header block here as well.
| ] | ||
| : []), | ||
| ]; | ||
| }, [appData, jsonSchema, loadingState]); |
There was a problem hiding this comment.
This tabs useMemo uses t(...) for labels (including the newly added Live Indexing tab), but t is not included in the dependency array. This can cause tab labels to not update when the UI language changes.
| "live-indexing": "Indexação ao vivo", | ||
| "load-more": "بارگذاری بیشتر", |
There was a problem hiding this comment.
This locale file is predominantly Persian, but the newly added live-indexing translation here is Portuguese. Please translate it to Persian so the locale remains consistent.
| ...(successContext?.stats?.vectorStats?.totalRecords | ||
| ? [ | ||
| { | ||
| title: t('label.vector-embedding-plural'), | ||
| dataIndex: 'vectorEmbeddings', |
There was a problem hiding this comment.
The vector-embeddings column is only enabled when successContext?.stats?.vectorStats?.totalRecords is truthy. This hides vector stats for failed runs (where only failureContext may be present) even though per-entity vectorSuccessRecords/vectorFailedRecords can still exist. Consider gating the column on either context (or on presence of vectorEmbeddings in the data) so the column appears consistently for failed runs too.
| "vector-embedding-plural": "Embeddings vetoriais", | ||
| "vector-stat-plural": "Vector Stats", |
There was a problem hiding this comment.
This locale file is predominantly Persian, but the newly added vector-embedding-plural translation here is Portuguese. Please translate it to Persian so the locale remains consistent.
| package org.openmetadata.service.apps; | ||
|
|
There was a problem hiding this comment.
New Java test files are expected to include the Apache 2.0 license header before the package declaration (see e.g. openmetadata-service/src/test/java/org/openmetadata/service/jdbi3/BulkExecutorTest.java:1-13). Please add the standard header block here as well.
| package org.openmetadata.service.resources.apps; | ||
|
|
||
| import static org.junit.jupiter.api.Assertions.assertEquals; | ||
| import static org.junit.jupiter.api.Assertions.assertNotNull; | ||
| import static org.junit.jupiter.api.Assertions.assertThrows; |
There was a problem hiding this comment.
New Java test file is missing the standard Apache 2.0 license header block before the package declaration (required across Java source/tests in this repo). Add the license header at the top of the file.
| package org.openmetadata.service.apps.scheduler; | ||
|
|
||
| import static org.junit.jupiter.api.Assertions.assertFalse; | ||
| import static org.junit.jupiter.api.Assertions.assertTrue; | ||
| import static org.mockito.ArgumentMatchers.any; |
There was a problem hiding this comment.
New Java test file is missing the standard Apache 2.0 license header block before the package declaration. Add the license header at the top of the file to match the repo convention.
| package org.openmetadata.service.apps; | ||
|
|
||
| import static org.junit.jupiter.api.Assertions.assertFalse; | ||
| import static org.mockito.Mockito.mock; | ||
|
|
There was a problem hiding this comment.
New Java test file is missing the standard Apache 2.0 license header block before the package declaration. Add the license header at the top of the file to match other Java sources/tests.
| Field unsafeField = Unsafe.class.getDeclaredField("theUnsafe"); | ||
| unsafeField.setAccessible(true); | ||
| Unsafe unsafe = (Unsafe) unsafeField.get(null); | ||
| appResource = (AppResource) unsafe.allocateInstance(AppResource.class); | ||
|
|
There was a problem hiding this comment.
This test uses sun.misc.Unsafe to instantiate AppResource, which is an internal JDK API and can break under stricter module access or future JDKs. Prefer constructing the resource normally (or via a test subclass) and injecting mocks via constructor/setters, or use reflection without Unsafe if necessary.
| export interface SearchIndexRetryRecord { | ||
| entityId: string; | ||
| entityFqn: string; | ||
| failureReason: string; | ||
| status: string; | ||
| entityType: string; | ||
| retryCount: number; | ||
| claimedAt: string | null; | ||
| } |
There was a problem hiding this comment.
claimedAt is backed by a java.sql.Timestamp on the server side; with the default Jackson settings this typically serializes as an epoch number, not a string. Align the UI type with the actual API payload (e.g., number | null, or a union if the API is intentionally stringified).
| // Verify vector-indexable entities get a number (0 when no vectorSuccessRecords in mock) | ||
| const tableEntry = resultData.find((e) => e.name === 'Table'); | ||
|
|
||
| expect(tableEntry?.vectorEmbeddings).toBe(0); | ||
|
|
||
| // Verify non-vector-indexable entities get null | ||
| const userEntry = resultData.find((e) => e.name === 'User'); | ||
|
|
||
| expect(userEntry?.vectorEmbeddings).toBeNull(); | ||
|
|
There was a problem hiding this comment.
This test validates the presence of vectorEmbeddings, but it never asserts the non-zero path where vectorSuccessRecords is provided. Add a case that includes a vector-indexable entity with vectorSuccessRecords set and assert that getEntityStatsData() maps it correctly, so the new behavior is actually covered by tests.
| @Min(0) | ||
| @Max(1000) | ||
| int limitParam, | ||
| @Parameter(description = "Offset records. (0 to 1000000, default = 0)") | ||
| @DefaultValue("0") | ||
| @QueryParam("offset") | ||
| @Min(0) | ||
| int offset) { |
There was a problem hiding this comment.
The validation annotations/descriptions are inconsistent: limit is documented as "1 to 1000" but @min(0) allows 0, and offset is documented as up to 1,000,000 but has no @max. Align the constraints (and/or descriptions) with the intended supported ranges to avoid confusing API consumers and potential large scans.
| @Min(0) | |
| @Max(1000) | |
| int limitParam, | |
| @Parameter(description = "Offset records. (0 to 1000000, default = 0)") | |
| @DefaultValue("0") | |
| @QueryParam("offset") | |
| @Min(0) | |
| int offset) { | |
| @Min(1) | |
| @Max(1000) | |
| int limitParam, | |
| @Parameter(description = "Offset records. (0 to 1000000, default = 0)") | |
| @DefaultValue("0") | |
| @QueryParam("offset") | |
| @Min(0) | |
| @Max(1000000) int offset) { |
| package org.openmetadata.service.jdbi3; | ||
|
|
||
| import static org.junit.jupiter.api.Assertions.assertEquals; | ||
| import static org.junit.jupiter.api.Assertions.assertFalse; | ||
| import static org.junit.jupiter.api.Assertions.assertTrue; |
There was a problem hiding this comment.
New Java file is missing the standard Apache 2.0 license header block before the package declaration. Add the license header to comply with the repo’s Java file convention (see existing Java sources/tests).
| package org.openmetadata.service.jdbi3; | ||
|
|
||
| import org.openmetadata.schema.type.Include; | ||
|
|
||
| /** |
There was a problem hiding this comment.
New Java source file is missing the standard Apache 2.0 license header block before the package declaration. Add the license header to comply with the repo’s Java file convention.
| "entity-distribution": "توزیع {{entity}}", | ||
| "entity-failed": "{{entity}} ناموفق", | ||
| "entity-feed-plural": "فیدهای نهاد", | ||
| "entity-fqn": "FQN da entidade", | ||
| "entity-hyphen-value": "{{entity}} - {{value}}", | ||
| "entity-id": "شناسه {{entity}}", | ||
| "entity-id-match": "مطابقت با شناسه", |
There was a problem hiding this comment.
The new translations added in this file appear to be Portuguese (e.g., "FQN da entidade", "Indexação ao vivo") while the rest of the locale content is Persian. Please replace these with correct Persian translations so the locale stays consistent.
| @@ -0,0 +1,33 @@ | |||
| package org.openmetadata.service.jdbi3; | |||
There was a problem hiding this comment.
This new Java file is missing the standard Apache 2.0 license header block before the package declaration. Please add the project’s license header at the top of the file to match repository conventions.
| @@ -0,0 +1,67 @@ | |||
| package org.openmetadata.service.jdbi3; | |||
There was a problem hiding this comment.
This new test file is missing the standard Apache 2.0 license header block before the package declaration. Please add the license header at the top to match repository conventions.
Code Review 🚫 Blocked 22 resolved / 29 findingsSearch index enhancement blocked due to 9 open critical and important findings including lost entity icon mappings (~30 EntityType keys), uncleaned auth login polling with up to 100 recursive timeouts, and empty MatillionDPC schema allowing invalid authentication. Additionally, 4 minor issues require resolution including buffer newline loss, repeated method calls on hot paths, and unsafe float overflow. 🚨 Bug: Entity icon mapping lost ~30 EntityType keys during refactorThe This will affect any component that calls Suggested fix
|
| Compact |
|
Was this helpful? React with 👍 / 👎 | Gitar
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 260 out of 263 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (2)
openmetadata-service/src/main/java/org/openmetadata/service/search/indexes/QueryCostRecordIndex.java:34
- The
catch (EntityNotFoundException)block logs "skipping indexing" but then rethrows the exception, which will still fail indexing for the QueryCostRecord. If the intent is to skip indexing when the referenced Query is missing, return the partially-built doc (or omit thequeryfield) instead of throwing; otherwise, update the log message to reflect that indexing will fail.
} catch (EntityNotFoundException ex) {
LOG.warn(
"Query entity [{}] not found for QueryCostRecord, skipping indexing: {}",
queryReference != null ? queryReference.getId() : "null",
ex.getMessage());
throw ex;
}
openmetadata-service/src/main/java/org/openmetadata/service/search/indexes/QueryCostRecordIndex.java:34
- The method logs "skipping indexing" when the referenced Query is missing, but then rethrows the exception, which will still fail indexing. Either actually skip (return the partial doc / omit the query fields) or change the log level/message and wrap into the expected indexing error handling.
} catch (EntityNotFoundException ex) {
LOG.warn(
"Query entity [{}] not found for QueryCostRecord, skipping indexing: {}",
queryReference != null ? queryReference.getId() : "null",
ex.getMessage());
throw ex;
}
| fetchRetryQueue(0); | ||
| }, | ||
| }} | ||
| paginationVisible={records.length > 0 && paging.total > pageSize} | ||
| rowKey={(record) => `${record.entityId}-${record.entityFqn}`} |
There was a problem hiding this comment.
Table component props (TableComponentProps) do not define a paginationVisible prop, so passing paginationVisible={...} here will fail TypeScript compilation. Use the existing pagination controls (pagination={false} / conditional pagination config, or customPaginationProps.showPagination) instead of an unsupported prop, or add/implement paginationVisible in the shared Table component if that's intended.
| "no-related-terms-available": "هیچ واژه مرتبطی موجود نیست.", | ||
| "no-relations-found": "رابطهای برای این اصطلاح یافت نشد", | ||
| "no-retry-queue-records": "Nenhum registro de tentativa de indexação ao vivo encontrado. Todas as entidades foram indexadas com sucesso.", | ||
| "no-roles-assigned": "هیچ نقشی اختصاص داده نشده است.", | ||
| "no-rule-found": "هیچ قانونی یافت نشد.", |
There was a problem hiding this comment.
This newly added message is in Portuguese in a Persian locale file. Please translate it to Persian (or use the project’s standard fallback language) so the empty state text is not shown in the wrong language.
| }, | ||
| }} | ||
| paginationVisible={records.length > 0 && paging.total > pageSize} | ||
| rowKey={(record) => `${record.entityId}-${record.entityFqn}`} |
There was a problem hiding this comment.
Table component props do not include paginationVisible, so this will fail TypeScript compilation (or be ignored at runtime). Use the existing customPaginationProps.showPagination pattern, or extend TableComponentProps to support paginationVisible consistently.
| App app = repository.getByName(uriInfo, name, repository.getFields("id")); | ||
| if (!"SearchIndexingApplication".equals(app.getName())) { | ||
| throw new BadRequestException( | ||
| "Live indexing queue is only available for SearchIndexingApplication"); | ||
| } |
There was a problem hiding this comment.
This endpoint returns live indexing retry queue records without any authorization check. Other AppResource actions call authorizer.authorize(...) with an appropriate OperationContext; please add an authorization check before returning queue data.
| // Verify basic structure | ||
| expect(resultData.length).toBeGreaterThan(0); | ||
|
|
||
| // Verify the result matches the sorted mock data | ||
| expect(resultData).toEqual(sortedMockData); | ||
| // Verify sorted by name | ||
| for (let i = 1; i < resultData.length; i++) { | ||
| expect( | ||
| resultData[i - 1].name.localeCompare(resultData[i].name) | ||
| ).toBeLessThanOrEqual(0); |
There was a problem hiding this comment.
This test no longer asserts the expected values for totalRecords/successRecords/failedRecords per entity, so regressions in the mapping/sorting logic could slip through. Consider keeping the previous full expected-array assertion and extend it to cover vectorEmbeddings for vector-indexable vs non-indexable entities.
|
|
Describe your changes:
Fixes #26678
I worked on ... because ...
Type of change:
Checklist:
Fixes <issue-number>: <short explanation>Summary by Gitar
BoundedListFilterto prevent out-of-range paginationSearchIndexExecutorerror handling with sink cleanup on failurequeryChangeDescriptioncheck in search index workflowBoundedListFilterTestfor boundary condition validationSearchIndexExecutorControlFlowTestwith additional control flow scenariostryStopOutsideQuartz()hook to handle stopping distributed jobs outside Quartz schedulerThis will update automatically on new commits.