fix(llm): restore the generic LLMCompletionClient layer (over-removed by #30041)#30102
Merged
Conversation
Contributor
❌ PR checklist incompleteThis PR cannot be merged until the following are addressed on its linked issue:
The fields live on the linked issue in the Shipping project (open the issue → right sidebar → Projects). After you set them, re-run this check (or push a commit) — issue/project changes do not re-trigger it automatically. Maintainers can bypass this check by adding the |
github-actions
Bot
added
Ingestion
safe to test
… extraction feature) The revert of #28973 (PR #30041) deleted the whole service.llm package, but #28973 introduced service.llm as a *generic, reusable* LLM completion layer (its own commit: "LLMCompletionClient layer ... Reusable by other platform features") — the runtime counterpart to the kept llmConfiguration/LlmConfigHolder. Downstream (Collate NLQ) reuses it, so deleting it broke the Collate build. Restore the generic layer (LLMCompletionClient, LLMClientHolder + startup init, CompletionResult/Options, the provider clients, Noop, factory, exception) and keep only the extraction feature removed (KnowledgePill and its ContextMemoryExtractor/ContextFileProcessingService consumers). LLMCompletionClientTest's pill-parsing cases now use a local PillLike record instead of KnowledgePill. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
pmbrull
force-pushed
the
pmbrull/restore-llm-completion-layer
branch
from
July 15, 2026 19:00
556d01f to
8873f1d
Compare
pmbrull
changed the title
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
Comment on lines
+5
to
+9
| /** | ||
| * Holds the single shared {@link LLMCompletionClient} built from {@code llmConfiguration} at | ||
| * startup, so downstream features (e.g. Context Center pill extraction) need not thread the config | ||
| * through every layer. Falls back to a {@link NoopCompletionClient} when unset or disabled. | ||
| */ |
There was a problem hiding this comment.
💡 Quality: Doc comments reference removed pill-extraction feature
This commit keeps the generic completion layer but removes the knowledge-pill extraction feature, yet the new Javadoc still ties the generic layer to it: LLMClientHolder says "downstream features (e.g. Context Center pill extraction)" and NoopCompletionClient says "Returns an empty pill array." Since the layer is now provider-neutral and reused by other features (e.g. Collate NLQ), reword these comments to drop the pill-specific references (e.g. "returns an empty JSON array") to avoid misleading future readers.
Was this helpful? React with 👍 / 👎
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+54
to
+59
| try { | ||
| parsed = parseArray(complete(systemPrompt, userPrompt).text(), elementType); | ||
| } catch (LLMCompletionException firstFailure) { | ||
| LOG.warn("LLM returned unparseable JSON; retrying once", firstFailure); | ||
| parsed = parseArray(complete(systemPrompt, userPrompt).text(), elementType); | ||
| } |
Contributor
There was a problem hiding this comment.
The catch includes both complete() and parseArray(), so authentication errors, rate limits, timeouts, and interruptions are treated as malformed JSON. A failed request is immediately repeated without backoff, doubling latency and potentially worsening provider throttling; only parsing failures should trigger the corrective retry.
Suggested change
| try { | |
| parsed = parseArray(complete(systemPrompt, userPrompt).text(), elementType); | |
| } catch (LLMCompletionException firstFailure) { | |
| LOG.warn("LLM returned unparseable JSON; retrying once", firstFailure); | |
| parsed = parseArray(complete(systemPrompt, userPrompt).text(), elementType); | |
| } | |
| CompletionResult completion = complete(systemPrompt, userPrompt); | |
| try { | |
| parsed = parseArray(completion.text(), elementType); | |
| } catch (LLMCompletionException firstFailure) { | |
| LOG.warn("LLM returned unparseable JSON; retrying once", firstFailure); | |
| parsed = parseArray(complete(systemPrompt, userPrompt).text(), elementType); | |
| } |
|
|
||
| public static synchronized void initialize(LLMConfiguration config) { | ||
| enabled = config != null && Boolean.TRUE.equals(config.getEnabled()); | ||
| instance = enabled ? LLMCompletionClientFactory.create(config) : new NoopCompletionClient(); |
Contributor
There was a problem hiding this comment.
A second initialization replaces the current client without closing it, and setForTesting has the same behavior. When the previous instance is a BedrockCompletionClient, application restarts in one JVM or test injection leave its AWS HTTP resources alive; replacement and application shutdown need a shared close path.
Context Used: CLAUDE.md (source)
Comment on lines
+65
to
+69
| } | ||
|
|
||
| @Override | ||
| protected CompletionResult doComplete( | ||
| String systemPrompt, String userPrompt, CompletionOptions options) { |
Contributor
There was a problem hiding this comment.
When an Azure endpoint and deployment are configured without apiVersion, String.format builds a URL ending in api-version=null. Every completion then reaches Azure with an invalid version and fails; validate this field or apply a supported default before constructing the endpoint.
Comment on lines
+85
to
+86
| } catch (IOException e) { | ||
| throw new LLMCompletionException("OpenAI completion failed due to IO error", e); |
Contributor
There was a problem hiding this comment.
The exception retains the complete external response body, and completeStructured logs that exception before retrying. If the provider echoes prompt content or account details in an error, those values are written to application logs; record the status and a sanitized provider error instead.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR continues the rollback of the knowledge-pill/AI cluster while restoring/keeping the generic LLM completion layer and reconciling MCP to be driven by the installed McpApplication (not the removed AI settings / MCP chat feature set).
Changes:
- Removes remaining AISettings + MCP Chat UI/backend/schema surfaces while keeping the underlying generic LLM completion client/test seam.
- Re-introduces MCP Server as an internal native application (
McpApplication) and updates MCP usage recording/reading to resolve the installed app instance. - Cleans up knowledge-pill extraction artifacts (request/filters, index mappings, UI badges/links, DB migration tables) that are no longer supported.
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| openmetadata-ui/src/main/resources/ui/src/utils/GlobalSettingsClassBase.ts | Removes AI settings entry from Global Settings menu |
| openmetadata-ui/src/main/resources/ui/src/utils/ApplicationSchemas/McpApplication.json | Adds MCP Server app configuration schema (origin validation) |
| openmetadata-ui/src/main/resources/ui/src/rest/settingConfigAPI.ts | Removes AISettings/MCP config client APIs and narrows settings type union |
| openmetadata-ui/src/main/resources/ui/src/rest/queries/metricQuery.ts | Updates metric default fields list (removes derived-from fields) |
| openmetadata-ui/src/main/resources/ui/src/rest/queries/glossaryTermQuery.ts | Updates glossary term default fields list (removes derived-from fields) |
| openmetadata-ui/src/main/resources/ui/src/rest/contextMemoryAPI.ts | Removes list params related to extraction source filtering |
| openmetadata-ui/src/main/resources/ui/src/pages/McpChatPage/NavItem.tsx | Deletes MCP Chat UI component |
| openmetadata-ui/src/main/resources/ui/src/pages/McpChatPage/NavItem.test.tsx | Deletes MCP Chat UI tests |
| openmetadata-ui/src/main/resources/ui/src/pages/McpChatPage/MessageList.tsx | Deletes MCP Chat UI component |
| openmetadata-ui/src/main/resources/ui/src/pages/McpChatPage/MessageList.test.tsx | Deletes MCP Chat UI tests |
| openmetadata-ui/src/main/resources/ui/src/pages/McpChatPage/ConversationSidebar.tsx | Deletes MCP Chat UI component |
| openmetadata-ui/src/main/resources/ui/src/pages/McpChatPage/ChatInput.less | Deletes MCP Chat styling |
| openmetadata-ui/src/main/resources/ui/src/pages/AISettingsPage/AISettingsPage.test.tsx | Deletes AI settings page test |
| openmetadata-ui/src/main/resources/ui/src/interface/knowledge-center.interface.ts | Removes page processing/extraction fields from UI interface |
| openmetadata-ui/src/main/resources/ui/src/generated/type/entityRelationship.ts | Removes DerivedFrom relationship type from generated enum |
| openmetadata-ui/src/main/resources/ui/src/generated/settings/settings.ts | Removes AISettings + related types from generated settings schema TS |
| openmetadata-ui/src/main/resources/ui/src/generated/entity/data/glossaryTerm.ts | Removes derivedFrom projection from glossary term type |
| openmetadata-ui/src/main/resources/ui/src/generated/entity/chat/mcpMessage.ts | Deletes MCP chat entity type TS |
| openmetadata-ui/src/main/resources/ui/src/generated/entity/chat/content/messageBlock.ts | Deletes MCP chat content TS |
| openmetadata-ui/src/main/resources/ui/src/generated/entity/chat/content/chatContentType.ts | Deletes MCP chat content TS |
| openmetadata-ui/src/main/resources/ui/src/generated/configuration/aiSettings.ts | Deletes AISettings configuration TS |
| openmetadata-ui/src/main/resources/ui/src/generated/api/chat/createMcpMessage.ts | Deletes MCP chat API request TS |
| openmetadata-ui/src/main/resources/ui/src/generated/api/chat/createMcpConversation.ts | Deletes MCP chat API request TS |
| openmetadata-ui/src/main/resources/ui/src/enums/sidebar.enum.ts | Removes MCP Chat sidebar enum entry |
| openmetadata-ui/src/main/resources/ui/src/enums/entity.enum.ts | Removes derived-* tab fields |
| openmetadata-ui/src/main/resources/ui/src/constants/GlobalSettings.constants.ts | Removes AI settings option constant |
| openmetadata-ui/src/main/resources/ui/src/constants/constants.ts | Removes MCP chat routes |
| openmetadata-ui/src/main/resources/ui/src/components/Settings/Applications/plugins/McpChatPlugin.ts | Deletes MCP Chat sidebar plugin |
| openmetadata-ui/src/main/resources/ui/src/components/Settings/Applications/ApplicationsProvider/ApplicationsProvider.tsx | Removes MCP chat enablement check + plugin injection |
| openmetadata-ui/src/main/resources/ui/src/components/Metric/MetricDetails/MetricDetails.tsx | Removes derived-from memory link rendering |
| openmetadata-ui/src/main/resources/ui/src/components/KnowledgeCenter/KnowledgePageDetailRightPanel/KnowledgePageDetailRightPanel.tsx | Removes extraction status + extracted memories UI for pages |
| openmetadata-ui/src/main/resources/ui/src/components/KnowledgeCenter/ArticleStatusBadge/ArticleStatusBadge.test.tsx | Deletes status badge tests |
| openmetadata-ui/src/main/resources/ui/src/components/KnowledgeCenter/ArticleStatusBadge/ArticleStatusBadge.interface.ts | Deletes status badge interface |
| openmetadata-ui/src/main/resources/ui/src/components/KnowledgeCenter/ArticleStatusBadge/ArticleStatusBadge.component.tsx | Deletes status badge component |
| openmetadata-ui/src/main/resources/ui/src/components/Glossary/GlossaryTerms/GlossaryTermsV1.component.tsx | Removes derived-from memory link rendering |
| openmetadata-ui/src/main/resources/ui/src/components/DataQuality/AddTestCaseList/AddTestCaseList.component.tsx | Adjusts empty-state layout for Add Test Case list |
| openmetadata-ui/src/main/resources/ui/src/components/ContextCenter/MemoryStatusBadge/MemoryStatusBadge.test.tsx | Deletes memory status badge tests |
| openmetadata-ui/src/main/resources/ui/src/components/ContextCenter/MemoryStatusBadge/MemoryStatusBadge.interface.ts | Deletes memory status badge interface |
| openmetadata-ui/src/main/resources/ui/src/components/ContextCenter/MemoryStatusBadge/MemoryStatusBadge.component.tsx | Deletes memory status badge component |
| openmetadata-ui/src/main/resources/ui/src/components/ContextCenter/DocumentsView/DocumentsView.component.tsx | Removes document status/memory-count UI elements |
| openmetadata-ui/src/main/resources/ui/src/components/ContextCenter/DocumentStatusBadge/DocumentStatusBadge.test.tsx | Deletes document status badge tests |
| openmetadata-ui/src/main/resources/ui/src/components/ContextCenter/DocumentStatusBadge/DocumentStatusBadge.interface.ts | Deletes document status badge interface |
| openmetadata-ui/src/main/resources/ui/src/components/ContextCenter/DocumentStatusBadge/DocumentStatusBadge.component.tsx | Deletes document status badge component |
| openmetadata-ui/src/main/resources/ui/src/components/ContextCenter/DerivedOntologyCard/DerivedOntologyCard.interface.ts | Deletes derived ontology card interface |
| openmetadata-ui/src/main/resources/ui/src/components/ContextCenter/CreateMemoryModal/CreateMemoryModal.test.tsx | Removes tests tied to deleted source-link/ontology UI |
| openmetadata-ui/src/main/resources/ui/src/components/ContextCenter/CreateMemoryModal/CreateMemoryModal.component.tsx | Simplifies memory source link handling; removes derived ontology UI |
| openmetadata-ui/src/main/resources/ui/src/components/AppRouter/SettingsRouter.tsx | Removes AI settings route |
| openmetadata-ui/src/main/resources/ui/src/components/AppRouter/AuthenticatedAppRouter.tsx | Removes MCP chat routes |
| openmetadata-ui/src/main/resources/ui/public/locales/en-US/Applications/McpChatApplication.md | Deletes MCP chat application docs |
| openmetadata-ui/src/main/resources/ui/playwright/e2e/Features/McpChat.spec.ts | Deletes MCP chat Playwright spec |
| openmetadata-spec/src/main/resources/json/schema/type/entityRelationship.json | Removes derivedFrom from relationship enum schema |
| openmetadata-spec/src/main/resources/json/schema/settings/settings.json | Removes AISettings from settings schema refs |
| openmetadata-spec/src/main/resources/json/schema/entity/data/glossaryTerm.json | Removes derivedFrom from glossary term schema |
| openmetadata-spec/src/main/resources/json/schema/entity/chat/mcpMessage.json | Deletes MCP message schema |
| openmetadata-spec/src/main/resources/json/schema/entity/chat/mcpConversation.json | Deletes MCP conversation schema |
| openmetadata-spec/src/main/resources/json/schema/entity/chat/content/messageBlock.json | Deletes MCP message block schema |
| openmetadata-spec/src/main/resources/json/schema/entity/chat/content/chatContentType.json | Deletes MCP content type schema |
| openmetadata-spec/src/main/resources/json/schema/configuration/aiSettings.json | Deletes AISettings schema |
| openmetadata-spec/src/main/resources/json/schema/api/chat/createMcpMessage.json | Deletes MCP create-message schema |
| openmetadata-spec/src/main/resources/json/schema/api/chat/createMcpConversation.json | Deletes MCP create-conversation schema |
| openmetadata-spec/src/main/resources/elasticsearch/en/context_memory_search_index.json | Removes sourceFile from context memory index mapping |
| openmetadata-spec/src/main/resources/elasticsearch/jp/context_memory_search_index.json | Removes sourceFile from context memory index mapping |
| openmetadata-spec/src/main/resources/elasticsearch/ru/context_memory_search_index.json | Removes sourceFile from context memory index mapping |
| openmetadata-spec/src/main/resources/elasticsearch/zh/context_memory_search_index.json | Removes sourceFile from context memory index mapping |
| openmetadata-service/src/test/java/org/openmetadata/service/util/AISettingsUtilTest.java | Deletes AI settings util tests |
| openmetadata-service/src/test/java/org/openmetadata/service/resources/system/AISettingsHandlerTest.java | Deletes AI settings handler tests |
| openmetadata-service/src/test/java/org/openmetadata/service/resources/mcp/McpUsageResourceTest.java | Updates test to mock ApplicationContext resolution |
| openmetadata-service/src/test/java/org/openmetadata/service/mcpclient/McpClientServiceTest.java | Deletes MCP client service tests |
| openmetadata-service/src/test/java/org/openmetadata/service/llm/LLMCompletionClientTest.java | Replaces KnowledgePill usage with a local test record |
| openmetadata-service/src/test/java/org/openmetadata/service/jdbi3/SystemRepositoryObjectStorageValidationTest.java | Deletes object storage validation test |
| openmetadata-service/src/test/java/org/openmetadata/service/EnumBackwardCompatibilityTest.java | Updates relationship enum expectations after removal |
| openmetadata-service/src/test/java/org/openmetadata/service/drive/memory/MemoryExtractorTest.java | Deletes memory agent extractor tests |
| openmetadata-service/src/test/java/org/openmetadata/service/drive/memory/MemoryDerivationTest.java | Deletes memory agent derivation parsing test |
| openmetadata-service/src/main/resources/json/data/settings/aiSettings.json | Deletes seeded AI settings |
| openmetadata-service/src/main/resources/json/data/botUser/memoryBot.json | Deletes memory bot user seed |
| openmetadata-service/src/main/resources/json/data/botUser/McpApplicationBot.json | Deletes MCP bot user seed |
| openmetadata-service/src/main/resources/json/data/bot/memoryBot.json | Deletes memory bot seed |
| openmetadata-service/src/main/resources/json/data/bot/McpApplicationBot.json | Deletes MCP bot seed |
| openmetadata-service/src/main/resources/json/data/appMarketPlaceDefinition/McpApplication.json | Adds marketplace definition for MCP Server native app |
| openmetadata-service/src/main/resources/json/data/app/McpApplication.json | Adds installed app seed for MCP Server |
| openmetadata-service/src/main/java/org/openmetadata/service/util/MemoryOwnership.java | Deletes automation-ownership helper for removed memory agent |
| openmetadata-service/src/main/java/org/openmetadata/service/util/AISettingsUtil.java | Deletes AI settings utility |
| openmetadata-service/src/main/java/org/openmetadata/service/search/vector/ContextMemoryBodyTextContributor.java | Removes source-file name from memory body-text contributor |
| openmetadata-service/src/main/java/org/openmetadata/service/search/indexes/ContextMemoryIndex.java | Removes sourceFile from indexed entity references |
| openmetadata-service/src/main/java/org/openmetadata/service/resources/settings/SettingsCache.java | Removes AISettings init; adjusts MCP config seeding behavior |
| openmetadata-service/src/main/java/org/openmetadata/service/resources/mcp/McpUsageResource.java | Resolves MCP app via ApplicationContext and handles uninitialized state |
| openmetadata-service/src/main/java/org/openmetadata/service/resources/drive/ContextFileResource.java | Switches to ContextFileExtractionService and removes recovery call |
| openmetadata-service/src/main/java/org/openmetadata/service/resources/context/ContextMemoryResource.java | Removes source filtering params; retains primaryEntity filtering |
| openmetadata-service/src/main/java/org/openmetadata/service/resources/context/ContextMemoryMapper.java | Stops mapping source file/entity onto created memories |
| openmetadata-service/src/main/java/org/openmetadata/service/resources/bots/BotResource.java | Removes bot impersonation grant logic tied to removed app bot role |
| openmetadata-service/src/main/java/org/openmetadata/service/OpenMetadataApplicationConfig.java | Reorders llmConfiguration field placement |
| openmetadata-service/src/main/java/org/openmetadata/service/OpenMetadataApplication.java | Initializes LLM holder; registers MCP server based on installed app |
| openmetadata-service/src/main/java/org/openmetadata/service/mcpclient/ToolExecutor.java | Deletes MCP chat tool executor interface |
| openmetadata-service/src/main/java/org/openmetadata/service/mcpclient/McpChatServiceHolder.java | Deletes MCP chat service holder |
| openmetadata-service/src/main/java/org/openmetadata/service/mcpclient/ClientDisconnectedException.java | Deletes MCP chat exception |
| openmetadata-service/src/main/java/org/openmetadata/service/mcpclient/ChatEventEmitter.java | Deletes MCP chat event emitter |
| openmetadata-service/src/main/java/org/openmetadata/service/mcpclient/ChatEvent.java | Deletes MCP chat event DTO |
| openmetadata-service/src/main/java/org/openmetadata/service/llm/KnowledgePill.java | Deletes knowledge-pill DTO (extraction feature removed) |
| openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/MetricRepository.java | Removes derived-from projection and automation ownership logic |
| openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/McpMessageRepository.java | Deletes MCP chat message repository |
| openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/McpConversationRepository.java | Deletes MCP chat conversation repository |
| openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/ListFilter.java | Removes context-memory source file/entity filter conditions |
| openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/GlossaryTermRepository.java | Removes derived-from projection and automation ownership logic |
| openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/GlossaryRepository.java | Removes automation ownership logic |
| openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/ContextFileRepository.java | Removes extraction-related field handling and child cleanup hooks |
| openmetadata-service/src/main/java/org/openmetadata/service/drive/PageContextProcessingEngineHolder.java | Deletes page extraction engine holder |
| openmetadata-service/src/main/java/org/openmetadata/service/drive/memory/MemoryVerdict.java | Deletes memory-agent DTO |
| openmetadata-service/src/main/java/org/openmetadata/service/drive/memory/MemoryRelation.java | Deletes memory-agent DTO |
| openmetadata-service/src/main/java/org/openmetadata/service/drive/memory/MemoryPromptBuilder.java | Deletes memory-agent prompt builder |
| openmetadata-service/src/main/java/org/openmetadata/service/drive/memory/MemoryExtractor.java | Deletes memory-agent LLM derivation step |
| openmetadata-service/src/main/java/org/openmetadata/service/drive/memory/MemoryDerivation.java | Deletes memory-agent DTO |
| openmetadata-service/src/main/java/org/openmetadata/service/drive/memory/MemoryContext.java | Deletes memory-agent grounding context |
| openmetadata-service/src/main/java/org/openmetadata/service/drive/memory/MemoryCandidate.java | Deletes memory-agent grounding DTO |
| openmetadata-service/src/main/java/org/openmetadata/service/drive/memory/MemoryAction.java | Deletes memory-agent action constants |
| openmetadata-service/src/main/java/org/openmetadata/service/drive/FileContextProcessingEngine.java | Deletes file extraction engine |
| openmetadata-service/src/main/java/org/openmetadata/service/drive/ContextProcessingEngine.java | Deletes extraction engine base |
| openmetadata-service/src/main/java/org/openmetadata/service/clients/llm/LlmToolCall.java | Deletes MCP-chat-specific LLM client types |
| openmetadata-service/src/main/java/org/openmetadata/service/clients/llm/LlmResponse.java | Deletes MCP-chat-specific LLM client types |
| openmetadata-service/src/main/java/org/openmetadata/service/clients/llm/LlmMessage.java | Deletes MCP-chat-specific LLM client types |
| openmetadata-service/src/main/java/org/openmetadata/service/clients/llm/LlmException.java | Deletes MCP-chat-specific LLM client types |
| openmetadata-service/src/main/java/org/openmetadata/service/clients/llm/LlmClientFactory.java | Deletes MCP-chat-specific LLM client factory |
| openmetadata-service/src/main/java/org/openmetadata/service/clients/llm/LlmClient.java | Deletes MCP-chat-specific LLM client interface |
| openmetadata-service/src/main/java/org/openmetadata/service/clients/llm/LlmConfigHolder.java | Updates docstring to reflect remaining use cases |
| openmetadata-service/src/main/java/org/openmetadata/service/attachments/AssetServiceFactory.java | Removes warning log when object storage is disabled |
| openmetadata-service/src/main/java/org/openmetadata/service/apps/bundles/mcp/McpApplication.java | Adds native MCP application class |
| openmetadata-service/src/main/java/org/openmetadata/service/apps/bundles/mcp/McpAppConstants.java | Simplifies constants now that app ID comes from installed app |
| openmetadata-mcp/src/main/java/org/openmetadata/mcp/usage/McpUsageRecorder.java | Resolves MCP app via ApplicationContext before recording usage |
| openmetadata-mcp/src/main/java/org/openmetadata/mcp/McpServer.java | Removes MCP client tool-executor wiring for deleted MCP chat |
| conf/openmetadata.yaml | Updates llmConfiguration comments to reflect current consumers |
| bootstrap/sql/migrations/native/2.0.0/postgres/schemaChanges.sql | Removes MCP chat message tracking tables |
| bootstrap/sql/migrations/native/2.0.0/postgres/postDataMigrationSQLScript.sql | Removes MCP chat/server post-migration logic for retired apps |
| bootstrap/sql/migrations/native/2.0.0/mysql/schemaChanges.sql | Removes MCP chat message tracking tables |
| bootstrap/sql/migrations/native/2.0.0/mysql/postDataMigrationSQLScript.sql | Removes MCP chat/server post-migration logic for retired apps |
| .gitignore | Adjusts ignored memory path rule |
Comments suppressed due to low confidence (1)
openmetadata-service/src/main/java/org/openmetadata/service/OpenMetadataApplication.java:454
- The check for whether MCP is initialized hardcodes the app name string. This risks drift from the canonical MCP app name used elsewhere (e.g.,
McpAppConstants.MCP_APP_NAME).
if (ApplicationContext.getInstance().getAppIfExists("McpApplication") != null) {
Comment on lines
+51
to
+61
| public final <T> List<T> completeStructured( | ||
| String systemPrompt, String userPrompt, Class<T> elementType) { | ||
| List<T> parsed; | ||
| try { | ||
| parsed = parseArray(complete(systemPrompt, userPrompt).text(), elementType); | ||
| } catch (LLMCompletionException firstFailure) { | ||
| LOG.warn("LLM returned unparseable JSON; retrying once", firstFailure); | ||
| parsed = parseArray(complete(systemPrompt, userPrompt).text(), elementType); | ||
| } | ||
| return parsed; | ||
| } |
Comment on lines
+43
to
+48
| boolean hasEndpoint = cfg.getEndpoint() != null && !cfg.getEndpoint().isBlank(); | ||
| boolean hasDeployment = cfg.getDeploymentName() != null && !cfg.getDeploymentName().isBlank(); | ||
| this.isAzure = hasEndpoint && hasDeployment; | ||
| this.endpoint = resolveEndpoint(cfg); | ||
| this.httpClient = HttpClient.newBuilder().connectTimeout(Duration.ofSeconds(30)).build(); | ||
| } |
Comment on lines
+80
to
+83
| if (response.statusCode() != 200) { | ||
| throw new LLMCompletionException( | ||
| "OpenAI API returned status " + response.statusCode() + ": " + response.body()); | ||
| } |
Comment on lines
+74
to
+77
| if (response.statusCode() != 200) { | ||
| throw new LLMCompletionException( | ||
| "Google API returned status " + response.statusCode() + ": " + response.body()); | ||
| } |
Comment on lines
+73
to
+76
| if (response.statusCode() != 200) { | ||
| throw new LLMCompletionException( | ||
| "Anthropic API returned status " + response.statusCode() + ": " + response.body()); | ||
| } |
Comment on lines
+16
to
+38
| public static synchronized void initialize(LLMConfiguration config) { | ||
| enabled = config != null && Boolean.TRUE.equals(config.getEnabled()); | ||
| instance = enabled ? LLMCompletionClientFactory.create(config) : new NoopCompletionClient(); | ||
| } | ||
|
|
||
| public static LLMCompletionClient get() { | ||
| LLMCompletionClient current = instance; | ||
| if (current == null) { | ||
| current = new NoopCompletionClient(); | ||
| } | ||
| return current; | ||
| } | ||
|
|
||
| public static boolean isEnabled() { | ||
| return enabled; | ||
| } | ||
|
|
||
| /** Test seam: inject a deterministic completion client (and force-enable) for integration tests. */ | ||
| public static synchronized void setForTesting(LLMCompletionClient client) { | ||
| instance = client; | ||
| enabled = client != null; | ||
| } | ||
| } |
|
Code Review
|
| Compact |
|
Was this helpful? React with 👍 / 👎 | Gitar
Contributor
🟡 Playwright Results — all passed (31 flaky)✅ 4537 passed · ❌ 0 failed · 🟡 31 flaky · ⏭️ 95 skipped
🟡 31 flaky test(s) (passed on retry)
How to debug locally# Download playwright-test-results-<shard> artifact and unzip
npx playwright show-trace path/to/trace.zip # view trace |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to the merged #30041.
Why
#30041 reverted the knowledge-pill extraction feature and, to do it, deleted the entire
openmetadata-service/.../service/llm/package. But #28973 introduced that package as a generic, reusable LLM completion layer — its own commit words:It's the runtime counterpart to the
llmConfiguration/LlmConfigHolderthat #30041 already kept. Collate's NLQ filter extraction reuses it (LLMCompletionClient,CompletionResult,LLMClientHolder,NoopCompletionClient), so deleting it broke the downstream Collate build:What (20 files, +1167)
Restore the generic layer; keep only the extraction feature removed.
LLMCompletionClient,LLMClientHolder(+ itsOpenMetadataApplicationstartup init fromllmConfiguration),CompletionResult,CompletionOptions,{Anthropic,Bedrock,Google,OpenAI}CompletionClient,NoopCompletionClient, factory, exception (+ unit tests).KnowledgePilland itsContextMemoryExtractor/ContextFileProcessingService/ Memory-Agent consumers.LLMCompletionClientTest's structured-parse cases use a localPillLikerecord instead ofKnowledgePill.Same "keep load-bearing infra, remove the feature" call already made for ContextMemory / llmConfiguration / objectStorage in #30041 — this layer was just missed.
Verified
service.llmunit tests +AIContextBuilderTest(persona canary) — 40/40 green;mvn spotless:applyclean.Follow-up
Bump Collate's OpenMetadata submodule to a
maincommit containing this fix for Collate CI to go green.🤖 Generated with Claude Code
Greptile Summary
This PR restores the reusable LLM completion layer while leaving knowledge-pill extraction removed. The main changes are:
Confidence Score: 4/5
The structured completion retry path can duplicate failed provider requests and should be fixed before merging.
LLMCompletionClient.java, LLMClientHolder.java, and OpenAICompletionClient.java
Security Review
Provider error bodies can flow into warning logs through the structured-completion retry path. Sanitizing these bodies would prevent prompt or account details returned by a provider from entering application logs.
Important Files Changed
Reviews (1): Last reviewed commit: "fix(llm): keep the generic LLMCompletion..." | Re-trigger Greptile
Context used (3)