kennknowles commented on code in PR #39980: URL: https://github.com/apache/beam/pull/39980#discussion_r3926451446
########## scripts/ci/pr-bot/shared/geminiReviewerAdvisor.ts: ########## @@ -0,0 +1,426 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { + PrHistoryContext, + CandidateContributor, + TouchedFileContext, +} from "./gitHistory"; + +/** + * Interface representing an individual recommended reviewer. + */ +export interface ReviewerRecommendation { + readonly username: string; + readonly role: "primary" | "secondary"; + readonly isCommitter: boolean; + readonly expertise: string; + readonly coveredFiles: readonly string[]; +} + +/** + * Interface representing an alternate reviewer suggestion. + */ +export interface AlternateReviewer { + readonly username: string; + readonly expertise: string; +} + +/** + * Result structure produced by the reviewer advisor. + */ +export interface ReviewerAdviceResult { + readonly selectedReviewers: readonly ReviewerRecommendation[]; + readonly alternateReviewers: readonly AlternateReviewer[]; + readonly reasoning: string; + readonly source: "gemini" | "heuristic-fallback"; +} + +/** + * Interface for LLM clients that can generate structured JSON. + */ +export interface IGeminiClient { + generateJson<T>(prompt: string): Promise<T>; +} + +/** + * Standard HTTP Gemini client using global fetch. + */ +export class GeminiClient implements IGeminiClient { + private readonly apiKey: string; + private readonly model: string; + + constructor(apiKey: string, model: string = "gemini-2.5-flash") { + this.apiKey = apiKey; + this.model = model; + } + + async generateJson<T>(prompt: string): Promise<T> { + if (!this.apiKey) { + throw new Error("GEMINI_API_KEY is not configured."); + } + + const url = `https://generativelanguage.googleapis.com/v1beta/models/${encodeURIComponent( + this.model + )}:generateContent?key=${encodeURIComponent(this.apiKey)}`; + + const response = await fetch(url, { + method: "POST", + headers: { + "Content-Type": "application/json", + }, + body: JSON.stringify({ + contents: [ + { + role: "user", + parts: [{ text: prompt }], + }, + ], + generationConfig: { + temperature: 0.1, + responseMimeType: "application/json", + }, + }), + }); + + if (!response.ok) { + const errorText = await response.text(); + throw new Error( + `Gemini API request failed with status ${response.status}: ${errorText}` + ); + } + + const data: any = await response.json(); + const candidateText = data?.candidates?.[0]?.content?.parts?.[0]?.text; + + if (!candidateText) { + throw new Error("Empty or invalid candidate response from Gemini API."); + } + + return JSON.parse(candidateText) as T; + } +} + +/** + * Configuration options for the Gemini Reviewer Advisor. + */ +export interface ReviewerAdvisorOptions { + readonly geminiClient?: IGeminiClient; + readonly committerCheck?: (username: string) => Promise<boolean>; + readonly exclusionList?: readonly string[]; + readonly maxReviewers?: number; +} + +/** + * Advisor that analyzes PR git history and selects optimal reviewers using Gemini or heuristic fallback. + */ +export class GeminiReviewerAdvisor { + private readonly client?: IGeminiClient; + private readonly committerCheck: (username: string) => Promise<boolean>; + private readonly exclusionList: readonly string[]; + private readonly maxReviewers: number; + + constructor(options: ReviewerAdvisorOptions = {}) { + this.client = options.geminiClient; + this.committerCheck = options.committerCheck ?? (async () => false); + this.exclusionList = options.exclusionList ?? []; + this.maxReviewers = options.maxReviewers ?? 2; + } + + /** + * Constructs the prompt instructing Gemini on how to select reviewers. + * + * @param context Extracted git and PR history. + * @param committers Map of username to committer status. + * @returns Detailed prompt string. + */ + public buildPrompt( + context: PrHistoryContext, + committers: Readonly<Record<string, boolean>> + ): string { + const fileSummaries = context.touchedFiles.map((file) => { + const commitSummaries = file.recentCommits + .slice(0, 5) + .map( + (c) => + ` - [${c.date}] ${c.authorLogin || c.authorName}: ${c.subject}` + ) + .join("\n"); + + return `- File: ${file.path} (+${file.additions}, -${ + file.deletions + }, changes: ${file.changes}${ + file.isNewFile ? " [NEW FILE]" : "" + })\n Recent Commits:\n${commitSummaries || " (No recent commits)"}`; + }); + + const candidateSummaries = context.candidates.map((c) => { + const isCommitter = committers[c.login] ?? false; + return `- @${c.login} (${c.name}): ${ + c.commitCount + } commits, last active ${ + c.lastCommitDate + }, committer=${isCommitter}. Files touched: ${c.touchedFilePaths.join( + ", " + )}`; + }); + + const exclusions = + this.exclusionList.map((e) => `@${e}`).join(", ") || "(none)"; + + return `You are the Apache Beam Code Review Assigner. +Your goal is to choose a small, optimal set of expert reviewers for a pull request based on real git history and file churn. Review Comment: Tried it... and yes it is better, but burns tokens for a while and lots of tool calls, plus I didn't attempt to restrict the context away from the existing PR comments and reviewers. The funniest result would be if it said "me! I'm the ultimate code reviewer assistant" but alas... # Suggest a good reviewer for https://github.com/apache/beam/pull/39156 The best reviewer for apache/beam#39156 https://github.com/apache/beam/pull/39156 is @ahmedabu98 (Ahmed Abualsaud). ────── ### Primary Recommendation: @ahmedabu98 (Ahmed Abualsaud) • Original Author: Authored WriteToPartitions.java https://github.com/apache/beam/blob/master/sdks/java/io/iceberg/src/main/java/org/apache/beam/sdk/io/iceberg/WriteToPartitions.java in PR #38061 https://github.com/apache/beam/pull/38061 when introducing hash distribution mode for Iceberg writes. • Component Lead: Primary maintainer and most active committer across the sdks/java/io/iceberg https://github.com/apache/beam/tree/master/sdks/java/io/iceberg module. • Context / Current Blocker: Ahmed already reviewed this PR on June 29 and left an inline design question on WriteToPartitions.java https://github.com/apache/beam/pull/39156#discussion_r3494205093: │ "Were you seeing OOMs even with autosharding enabled? AFAIK autosharding should distribute to different keys, so the state for any one key shouldn't get too large. If it makes sense, we can keep │ the GroupIntoBatches.withShardedKey() path for autosharding batch writes. Otherwise maybe we can add a num_shards config to do fixed sharding per partition, and stick with GBK" The PR is currently stalled awaiting the author's response to this question, which has caused the Beam PR bot to repeatedly cycle reviewers (@ahmedabu98 ↔ @chamikaramj ↔ @Abacn). Once addressed, @ahmedabu98 is best positioned to approve and merge it. ────── ### Alternative Reviewers 1. @chamikaramj (Chamikara Jayalath) • Beam PMC member and senior I/O specialist. • Reviewed the original PR #38061 https://github.com/apache/beam/pull/38061 implementing WriteToPartitions. • Deep expertise in Beam core transforms, batch vs. streaming execution semantics, and runner shuffle behavior (GroupByKey vs. GroupIntoBatches). 2. @clairemcginty (Claire McGinty) • Active contributor to IcebergIO, having recently modified WriteToPartitions.java https://github. com/apache/beam/blob/master/sdks/java/io/iceberg/src/main/java/org/apache/beam/sdk/io/iceberg/WriteToPartitions.java in PR #39250 https://github.com/apache/beam/pull/39250. • Colleague of the PR author at Spotify, with direct context on large-scale production Iceberg write pipelines. # Suggest a good reviewer for https://github.com/apache/beam/pull/37535 Considering Stream Completion Logic Based on the changes in PR #37535 https://github.com/apache/beam/pull/37535 ("Fix temp table cleanup for continuous read in BigQueryIO" by @stankiewicz), here are the best reviewers to suggest: ────── ### Primary Recommendations #### 1. Yi Hu (@Abacn https://github.com/Abacn) — Recommended primary reviewer • Why: • Already active on this PR: Yi previously reviewed this PR in April 2026 and requested a load test regarding the overhead of the stateful CleanupTempTableDoFn, as well as handling the zero- stream edge case. • Addressed feedback: The author (Radek) has since conducted the benchmark, posted the load test doc https://docs.google.com/document/d/1yUxqlv2118xc-6AN- 8OxFgBi9iZRg0HcDToVElWoetY/edit?usp=sharing, and added tests for zero streams. • Code ownership: Designated reviewer for the Java label and frequent contributor to BigQueryStorageStreamSource and BigQuery IO. • Next step: A ping to Yi to review the benchmark results and verify the zero-stream edge case test. #### 2. Chamikara Jayalath (@chamikaramj https://github.com/chamikaramj) — Recommended for runner/fault-tolerance validation • Why: • Pending review thread: Chamikara reviewed the PR in July 2026 and raised a critical question regarding runner failure and retry semantics: │ "Should there be a shuffle between 'ReadDynamicStreamSource' and 'CleanupTempTableDoFn'? What if there's a runner failure of the downstream output processing and a retry after cleanup │ operation already performed? ... Just want to make sure that we considered and ruled out the case of premature deletion of temp tables before they are fully processed and corresponding │ workitems are completed." • Radek replied on July 23 noting that CleanupTempTableDoFn is a stateful DoFn (introducing a key-by shuffle on the cleanup messages), but Chamikara has not yet weighed in on whether that fully guarantees downstream consumers won't attempt a re-read if an uncheckpointed bundle fails. • Chamikara's sign-off is essential to ensure this doesn't risk data loss under runner worker failure scenarios. ────── ### Secondary / Alternative Reviewers #### 3. Danny McCormick (@damccorm https://github.com/damccorm) • Why: • Danny reviewed and approved Radek's original PRs that implemented this entire feature: • #36668 https://github.com/apache/beam/pull/36668: Streaming read for BigQuery • #36912 https://github.com/apache/beam/pull/36912: Fix retry scenario for query to table materialization • He already possesses full context on the design and lifecycle of BigQuery dynamic streaming reads. #### 4. Reuven Lax (@reuvenlax https://github.com/reuvenlax) • Why: • Primary architect and maintainer of BigQueryIO and Dataflow streaming/stateful semantics (including the original batch temp table cleanup transform PassThroughThenCleanup). • If there is any remaining question about the stateful DoFn approach vs. stream completion guarantees across runners, Reuven's review would be authoritative. ────── ### Suggested Action Since @Abacn and @chamikaramj are already assigned and have pending review threads on the PR, the best step is to re-ping both of them on the PR: R: @Abacn @chamikaramj Specifically highlighting that the load test results and the response to the runner-retry question are ready for their re-evaluation. # Suggest a good reviewer for https://github.com/apache/beam/pull/39363 Examining artifact staging issues The recommended reviewer for apache/beam#39363 https://github.com/apache/beam/pull/39363 is: ### Primary Recommendation: @Abacn (Yi Hu) Why: • Active Engagement & Verification: @Abacn is already involved with this PR. They verified the changes on Windows (running Spark portable runner tests) and commented on the PR. • Direct Context on Artifact Staging & Windows: They reviewed and merged the closely related PR #39367 https://github.com/apache/beam/pull/39367 (handling InvalidPathException during artifact staging on Windows) and subsequently opened #39916 https://github.com/apache/beam/pull/39916 addressing the broader Windows artifact staging filename and CI fixes. • Committer Status: As an Apache Beam committer and PMC member, @Abacn has the necessary permissions to guide the PR to completion and merge it once open comments are resolved. ────── ### Domain Expert Alternative: @robertwb (Robert Bradshaw) • Why: Robert is the original author of ArtifactStagingService.java https://github.com/apache/beam/blob/master/runners/java-fn- execution/src/main/java/org/apache/beam/runners/fnexecution/artifact/ArtifactStagingService.java and leads the Fn Execution / Portability artifact management architecture. He is the best escalation point if there are questions about cross-language artifact retrieval contracts or naming semantics across SDKs. ────── ### Context on Existing Reviews • @Eliaaazzz (Elia Liu) already performed an in-depth review on this PR, pointing out that the Splitter pattern ([^A-Za-z-_.]]) contains a stray bracket causing base to carry unescaped characters, and suggesting handling of control characters. While not currently a committer with merge bits, their feedback is the primary blocker @Abacn cited as needing resolution before merge. -- 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]
