chore: add signature validation listing command + correct hour-based GCP lookup & local comparison logic #1902
Conversation
rockysingh
force-pushed
the
1890-gcp-bucket-sampling-ls-command
branch
from
8d298d2 to
208741a
Compare
rockysingh
force-pushed
the
1890-gcp-bucket-sampling-ls-command
branch
from
5e5a219 to
844cb05
Compare
Nana-EC
left a comment
Nana-EC
left a comment
There was a problem hiding this comment.
Nice updates.
Some suggestions where applicable
| * Builds X and returns Y. | ||
| * | ||
| * | ||
|
|
There was a problem hiding this comment.
Q: was this just formatting change, wonder what prompted this.
Might either fix with ./gradlew spotlessApply or revert
There was a problem hiding this comment.
This was caused by a rebase. I will try the ./gradlew spotlessApply
| @Option( | ||
| names = "--skip-local-compare", | ||
| description = "Skip comparing against local downloadedDays dir; only show GCP signature counts") | ||
| private boolean skipLocalCompare = false; |
There was a problem hiding this comment.
Some options have default in the Command option and others in the initialization.
Highlighting in case there's a desired approach you're going for to convey optional params and making it visible to the user in the command line what defaults are.
There was a problem hiding this comment.
I've added all the defaults to the options for the command.
| @Option(names = "--end-date", required = true, description = "End date (inclusive), yyyy-MM-dd") | ||
| private LocalDate endDate; | ||
|
|
||
| @Option(names = "--samples", description = "Number of random (day, hour) samples", defaultValue = "10") |
There was a problem hiding this comment.
Might be worth considering a max to not overwhelm a user.
There was a problem hiding this comment.
Good shout. I've added the max.
| "%-8s %-10d %-10d %-10d %-8s%n", "0.0." + r.node, r.gcpSigCount, r.localSigCount, r.diff, status); | ||
| } | ||
|
|
||
| printDifferenceView(results); |
There was a problem hiding this comment.
Might be worth a reorder printView(List<NodeResult> results) which iterates over results and then calls printDifferenceView(NodeResult result) and printLsView(NodeResult result) based on appropriate flags.
This allows for a single loop of results which could be useful is samples are large
| */ | ||
| private NodeResult checkNodeForHour(LocalDate date, int hour, int node) { | ||
| List<String> gcpSigFiles = List.of(); | ||
| long gcpSigCount; |
There was a problem hiding this comment.
For clarity I think you should default the counts and diff to 0, then set them to -1 on failures or skips (skips should be clear as the printouts should input)
Then in the else that sets localRecordFiles below it should be an if else conditions.
In this way you can have the logic fail fast and skip certain lines
rockysingh
force-pushed
the
1890-gcp-bucket-sampling-ls-command
branch
from
d18e5bd to
7601756
Compare
| cacheEnabled, cacheDir.toPath(), minNodeAccountId, maxNodeAccountId, userProject); | ||
| } | ||
| if (!skipLocalCompare && downloadedDaysDir == null) { | ||
| throw new NotFoundException("DownloadedDaysDir has not been set"); |
There was a problem hiding this comment.
Just use an IllegalArgumentException here also as this only happens when downloadedDaysDir is invalid.
Seems too much to add helidon NotFoundException just for this
rockysingh
force-pushed
the
1890-gcp-bucket-sampling-ls-command
branch
from
7f5a5eb to
f2be5ac
Compare
jasperpotts
left a comment
jasperpotts
left a comment
There was a problem hiding this comment.
There is a core design issue here that is wrong, you should be doing validation for all nodes at a simple time in one go.
| List<String> localRecordFiles = List.of(); | ||
|
|
||
| try { | ||
| var blobs = mainNetBucket.listHour(date.toEpochDay()); |
There was a problem hiding this comment.
should this not be passing in false for not including sidecar files?
| List<String> localRecordFiles = List.of(); | ||
|
|
||
| try { | ||
| var blobs = mainNetBucket.listHour(date.toEpochDay()); |
There was a problem hiding this comment.
"blobs" is a confusing var name as they are chain files. I assumed the were blobs from GCP SDK to start with
There was a problem hiding this comment.
Yep blobs is what gcp refers to them. I will rename it to more appropriate name.
| gcpSigFiles = blobs.stream() | ||
| .map(b -> b.path()) | ||
| .filter(path -> path.contains(hourToken)) | ||
| .filter(this::isRecordName) |
There was a problem hiding this comment.
this is wrong isRecordName looks for ".rcd" or ".rcd.gz". Signature files have names like "2019-09-13T21_53_51.396440Z.rcd_sig"
| .map(b -> b.path()) | ||
| .filter(path -> path.contains(hourToken)) | ||
| .filter(this::isRecordName) | ||
| .filter(path -> matchesNode(path, node)) |
There was a problem hiding this comment.
You are also filtering to a single node, so you would only ever see 1 signature file. Seems like you want all nodes not just one node?
| .map(this::extractFileName) | ||
| .toList(); | ||
|
|
||
| gcpSigCount = gcpSigFiles.size(); |
There was a problem hiding this comment.
this will almost always be 1 and will tell you there is a single record file not that there is a signature file.
| names = "--skip-local-compare", | ||
| description = "Skip comparing against local downloadedDays dir; only show GCP signature counts", | ||
| defaultValue = "false") | ||
| private boolean skipLocalCompare; |
There was a problem hiding this comment.
Seems mostly pointless as aim of command was to compare, a random gcp ls command is not very useful
| handleSample(i + 1, sampleDate, hour, executor); | ||
| } | ||
| } finally { | ||
| executor.shutdownNow(); |
There was a problem hiding this comment.
This will just terminate all background jobs in executor
There was a problem hiding this comment.
ok I see, handleSample blocks on future.get() So you have parallelism across nodes but not samples. Feel you can move threading up a level and do multiple samples in parallel.
|
|
||
| // submit node tasks in parallel | ||
| final List<Future<NodeResult>> futures = new ArrayList<>(); | ||
| for (int node = 0; node < nodeCount; node++) { |
There was a problem hiding this comment.
Not sure why you look node's here. Should do all nodes at once. A single GCP ls using org.hiero.block.tools.utils.gcp.MainNetBucket.listHour will include all nodes. That is one of the features of MainNetBucket class.
|
|
||
| List<String> gcpOnly = List.of(); | ||
| List<String> localOnly = List.of(); | ||
| List<String> localRecordFiles = List.of(); |
There was a problem hiding this comment.
build a Map<String"Timestamp" or Instant, Integer count of sig files> of record file time to sig count
| * then filters blobs for this node and record file extensions. | ||
| * - Local: counts *.rcd / *.rcd.gz files in downloadedDaysDir for date+hour+node. | ||
| */ | ||
| private NodeResult checkNodeForHour(LocalDate date, int hour, int node) { |
There was a problem hiding this comment.
change to per hour only not per node
| var blobs = mainNetBucket.listHour(date.toEpochDay()); | ||
| final String hourToken = date + "T" + String.format("%02d", hour) + "_"; | ||
|
|
||
| gcpSigFiles = blobs.stream() |
There was a problem hiding this comment.
change to filter to sig files only, grouopby timestamp, then count and produce map.
| } | ||
|
|
||
| // handle skip vs non-skip for local compare | ||
| if (skipLocalCompare) { |
There was a problem hiding this comment.
always compare, scan whole day using same code as in org.hiero.block.tools.days.subcommands.Ls.run then take stream of day, filter to only hour you care about , group by, count, compare , record results. So ideally you want to know how many were the same and then keep track of all different and print.
rockysingh
force-pushed
the
1890-gcp-bucket-sampling-ls-command
branch
from
3201d3c to
904b92c
Compare
|
|
||
| try { | ||
| // Scan the whole day and then filter to the hour, as requested in review. | ||
| var chainFilesList = mainNetBucket.listHour(date.toEpochDay(), false); |
There was a problem hiding this comment.
this is wrong, the input to listHour is a block time in nanos since stream start 2019. You are also not using the "hour" part just day. See org.hiero.block.tools.records.RecordFileDates.instantToBlockTimeLong you need to convert date + hour to Instant then pass to that method.
There was a problem hiding this comment.
I’ve updated the implementation so we now convert the sampled (date, hour) into a UTC Instant and then into a block time in nanos using RecordFileDates.instantToBlockTimeLong(...), and pass that to MainNetBucket.listHour(..).
| return idx >= 0 ? path.substring(idx + 1) : path; | ||
| } | ||
|
|
||
| static final class SigSummary { |
| } | ||
| } | ||
|
|
||
| static final class PerTimestampDiff { |
There was a problem hiding this comment.
this can be a "record" too
| } | ||
| } | ||
|
|
||
| static final class HourResult { |
| return new HourResult(date, hour, same, diffs); | ||
| } | ||
|
|
||
| private String extractFileName(String path) { |
There was a problem hiding this comment.
See org.hiero.block.tools.records.RecordFileDates.extractRecordFileTime, can just use that I expect
| // Scan the whole day and then filter to the hour, as requested in review. | ||
| var chainFilesList = mainNetBucket.listHour(date.toEpochDay(), false); | ||
|
|
||
| chainFilesList.stream() |
There was a problem hiding this comment.
I think this could just be returning a Map<Instant,Long> blockTimeToCount with code like.
Map<Instant, Long> blockTimeToCount = chainFilesList.stream()
.filter(this::isSignatureName)
.collect(Collectors.groupingBy(
RecordFileDates::extractRecordFileTime, // Extract block time
Collectors.counting() // Count occurrences
));
no need to filter by hour as listHour should only give you hour needed
|
|
||
| final List<Path> dayPaths = TarZstdDayUtils.sortedDayPaths(compressedDayOrDaysDirs); | ||
| for (Path dayFile : dayPaths) { | ||
| try (var stream = TarZstdDayReaderUsingExec.streamTarZstd(dayFile)) { |
There was a problem hiding this comment.
Should be able to similar groupBy here.
| * Build a summary of record timestamp -> count of signature files in the local compressed days | ||
| * and the list of underlying local signature file names for a given date and hour. | ||
| */ | ||
| protected SigSummary buildLocalSigSummary(LocalDate date, int hour) { |
There was a problem hiding this comment.
Think for both SigSummary can just be Map<Instant, Long>
There was a problem hiding this comment.
I've removed SigSummary and use the map now.
| * Compare GCP vs local signature counts for a given hour, grouped by record timestamp. | ||
| */ | ||
| protected HourResult compareHour(LocalDate date, int hour) { | ||
| // NOTE: --skip-local-compare currently ignored; always comparing GCP vs local as per review feedback. |
There was a problem hiding this comment.
you could just take the two Map<Instant, Long> maps. Compare the keys first to make sure they have same set of keys. Then for each key compare the counts.
rockysingh
changed the title
gcp-ls sampling command with optional LS view for GCP record listings
rockysingh
force-pushed
the
1890-gcp-bucket-sampling-ls-command
branch
5 times, most recently
from
8349bcb to
3d0ee14
Compare
Signed-off-by: Rocky Thind <harpender.t@swirldslabs.com>
Signed-off-by: Rocky Thind <harpender.t@swirldslabs.com>
…lexity Signed-off-by: Rocky Thind <harpender.t@swirldslabs.com>
Signed-off-by: Rocky Thind <harpender.t@swirldslabs.com>
Signed-off-by: Rocky Thind <harpender.t@swirldslabs.com>
…nNetBucket, and simplify per-node result handling (fail-fast counts, single-loop print with clearer diff/ls views). Signed-off-by: Rocky Thind <harpender.t@swirldslabs.com>
Signed-off-by: Rocky Thind <harpender.t@swirldslabs.com>
Signed-off-by: Rocky Thind <harpender.t@swirldslabs.com>
Signed-off-by: Rocky Thind <harpender.t@swirldslabs.com>
Signed-off-by: Rocky Thind <harpender.t@swirldslabs.com>
Signed-off-by: Rocky Thind <harpender.t@swirldslabs.com>
… local signature files at the timestamp level and surface full file lists for mismatches. The command now uses SigSummary for both GCP and local sources, aggregates sig counts properly, and prints detailed per-timestamp differences. Updated visibility of helper classes/methods so the logic can be cleanly unit-tested without GCP/tar.zst dependencies. Signed-off-by: Rocky Thind <harpender.t@swirldslabs.com>
Signed-off-by: Rocky Thind <harpender.t@swirldslabs.com>
…istingCommand to align with review feedback and ensure correctness. We now explicitly compare the key sets from GCP and local summaries: any timestamp missing on either side is treated as a mismatch, and only timestamps present in both maps with identical counts are marked as “same.” The PerTimestampDiff record typo was fixed, and all relevant Javadocs were updated to reflect the new behaviour and clarify how record times and signature counts are derived from GCP listings and compressed day archives. No output formatting changes were made; this improves accuracy, determinism, and readability of the hour-level signature validation. Signed-off-by: Rocky Thind <harpender.t@swirldslabs.com>
Signed-off-by: Rocky Thind <harpender.t@swirldslabs.com>
…ing record time Signed-off-by: Rocky Thind <harpender.t@swirldslabs.com>
Signed-off-by: Rocky Thind <harpender.t@swirldslabs.com>
Signed-off-by: Rocky Thind <harpender.t@swirldslabs.com>
Signed-off-by: Rocky Thind <harpender.t@swirldslabs.com>
Signed-off-by: Rocky Thind <harpender.t@swirldslabs.com>
rockysingh
force-pushed
the
1890-gcp-bucket-sampling-ls-command
branch
from
d95c614 to
f77fb43
Compare
Signed-off-by: Rocky Thind <harpender.t@swirldslabs.com>
rbarker-dev
left a comment
rbarker-dev
left a comment
There was a problem hiding this comment.
Changes to the gradle and markdown files look okay to me.
4 participants
#Summary
This PR introduces the new sig-validation-ls command and completes the design corrections requested during review. The command randomly samples (date, hour) ranges and compares GCP signature counts against locally derived counts from compressed .tar.zst day archives.
Key Changes
• Implemented sig-validation-ls command to perform hour-level signature validation across sampled days.
• Corrected GCP lookup logic:
(date + hour) is now converted to a UTC Instant and then to block-time nanos via
RecordFileDates.instantToBlockTimeLong, ensuring MainNetBucket.listHour receives the correct input.
• Reworked comparison logic (compareHour):
• Explicit union of GCP + local timestamp keys
• Missing timestamps are treated as mismatches
• Only equal counts on both sides increment the same counter
• Fixed PerTimestampDiff record and ensured consistent usage throughout the command.
• Updated and clarified Javadocs to match the new behaviour and better explain how timestamps and signature counts are derived.
• Added a clean, deterministic test that injects synthetic GCP and local summaries without relying on filesystem IO.
Why This Matters
This corrects the core design flaw where signature validation was based only on the day rather than the specific hour. It also aligns the logic with expected behaviour for detecting incomplete or mismatched signature sets across GCP and local compressed archives.
Testing
• Added unit test for compareHour using controlled synthetic input.
• Manually validated against 2019-09-13 and 2019-09-14 compressed days.