Add compact index support for private sources #392

segiddins · 2024-07-09T19:12:44Z

Description:

Ensured working via https://github.com/rubygems/gem_server_conformance

Blocked by Update sqlite-jdbc to 3.46 jruby/activerecord-jdbc-adapter#1149
2 postgres integration specs failing on CI only

Tasks:

Describe the problem / feature
Write tests

I will abide by the code of conduct.

indirect

looking good!

I'm also interested to hear if this gave you any ideas about how we could improve the compact index gem for consumers, so their implementations could potentially be shorter/easier.

lib/gemstash/compact_index_builder.rb

technicalpickles · 2024-07-31T20:17:48Z

Is there any extra configuration needed to enable this? Wanted to test locally, but see this when trying to bundle install against it:

HTTP GET http://localhost:9292/private/versions
HTTP 404 Not Found http://localhost:9292/private/versions
Bundler::Fetcher::FallbackError: Gem::Net::HTTPNotFound: http://localhost:9292/private/versions

segiddins · 2024-07-31T20:27:38Z

@technicalpickles shouldn't be, what's showing up in the server logs?

technicalpickles · 2024-07-31T21:11:36Z

Startup:

❯ bin/gemstash start                                                                                                                                                                      ─╯
Starting gemstash!
[2024-07-31 17:10:38 -0400] - INFO - [19490] Puma starting in cluster mode...
[2024-07-31 17:10:38 -0400] - INFO - [19490] * Puma version: 6.4.2 (ruby 3.3.2-p78) ("The Eagle of Durango")
[2024-07-31 17:10:38 -0400] - INFO - [19490] *  Min threads: 0
[2024-07-31 17:10:38 -0400] - INFO - [19490] *  Max threads: 16
[2024-07-31 17:10:38 -0400] - INFO - [19490] *  Environment: development
[2024-07-31 17:10:38 -0400] - INFO - [19490] *   Master PID: 19490
[2024-07-31 17:10:38 -0400] - INFO - [19490] *      Workers: 1
[2024-07-31 17:10:38 -0400] - INFO - [19490] *     Restarts: (✔) hot (✔) phased
[2024-07-31 17:10:38 -0400] - INFO - [19490] * Listening on http://0.0.0.0:9292
[2024-07-31 17:10:38 -0400] - INFO - [19490] Use Ctrl-C to stop
[2024-07-31 17:10:38 -0400] - INFO - [19490] ! WARNING: Detected running cluster mode with 1 worker.
[2024-07-31 17:10:38 -0400] - INFO - [19490] ! Running Puma in cluster mode with a single worker is often a misconfiguration.
[2024-07-31 17:10:38 -0400] - INFO - [19490] ! Consider running Puma in single-mode (workers = 0) in order to reduce memory overhead.
[2024-07-31 17:10:38 -0400] - INFO - [19490] ! Set the `silence_single_worker_warning` option to silence this warning message.
/Users/josh.nichols/workspace/gemstash/lib/gemstash/web.rb:10: warning: Skipping set of ruby2_keywords flag for initialize (method accepts keywords or method does not accept argument splat)
[2024-07-31 17:10:38 -0400] - INFO - [19490] - Worker 0 (PID: 19492) booted in 0.36s, phase: 0

During bundle update bigdecimal --conservative:

[2024-07-31 17:10:47 -0400] - INFO - Rewriting '/private/versions' to '/versions'
[2024-07-31 17:10:47 -0400] - INFO - Rewriting '/private/api/v1/dependencies' to '/api/v1/dependencies'
[2024-07-31 17:10:47 -0400] - INFO - Rewriting '/private/api/v1/dependencies?gems=redacted-gem' to '/api/v1/dependencies?gems=redacted-gem'
[2024-07-31 17:10:47 -0400] - INFO - Querying dependencies: redacted-gem
[2024-07-31 17:10:47 -0400] - INFO - Rewriting '/private/api/v1/dependencies?gems=bigdecimal' to '/api/v1/dependencies?gems=bigdecimal'
[2024-07-31 17:10:47 -0400] - INFO - Querying dependencies: bigdecimal

segiddins · 2024-09-17T21:50:36Z

I'll revisit this once there's a version of sqlite for jruby that supports what I used here

martinemde · 2025-02-25T04:27:27Z

Hey @segiddins, the jdbc-sqlite3 seems like a long term blocker at this point. Is the performance greatly affected by not having string_agg? I'd be willing to fix it up to see if we can get this in if you don't have time, but I could use some context about that choice.

segiddins · 2025-02-25T07:08:03Z

It's the only way to get an array out of SQLite unfortunately. I wish we knew what jruby usage was like :/

technicalpickles · 2025-02-25T14:27:19Z

Would it feasible to disable compact index on jruby until the blocker is fixed? That way, non-jruby users would get the benefit. We could also keep a PR open to enable it on jruby, so we have a reminder and/or place to validate fixes.

simi · 2025-02-25T18:25:57Z

CI logs are not reachable anymore. Would you mind to rebase this and update deps? Seems there are some news at jruby/activerecord-jdbc-adapter#1149 (comment).

martinemde · 2025-02-27T06:54:35Z

When I was looking closely at JRuby compact index results, they were coming out ordered differently even with identical code (and even aggregated as a separate query with its own sort) so I suspect jdbc-sqlite3 is not behaving identically to cruby's adapters. The order might be because of time field precision differences since we sort by created_at. Just a hunch.

martinemde · 2025-03-06T19:01:23Z

What do you think of skipping conformance tests for jruby and releasing compact index as experimental on jruby for now? We can see it's passing for most other rubies, and with the conformance tests working, we have a reasonably high confidence that this is a working implementation. We also don't know if anyone runs their gemstash as jruby, but we could wait for feedback to try to diagnose these issues if we find people need it.

segiddins · 2025-03-10T21:53:26Z

lib/gemstash/migrations/06_compact_index.rb

+
+Sequel.migration do
+  change do
+    alter_table :versions do # TODO: backfill info_checksum, sha256, required_ruby_version, required_rubygems_version


@indirect any thoughts on how best to do this, now that the PR is passing?

Should we add a gemstash update or gemstash sync command that would be a standard idempotent way to reindex/update after a new version is released?

Have we done a migration like this before?

yeah, I think a command to migrate or update is the way to go, with some docs about how to update to the newer version.

Are we sure we need an explicit command? I found Gemstash::Env.migrate which handles running migrations. That is called in Gemstash::Env.db, so basically anytime the db is accessed it is automatically migrated before rturning a connection.

edit: I see now, it's more about the backfill than the migration.

I suspect we'll need to do something to detect if the backfill has been done, and disable compact indexes with a warning. Tht'd be a good place to tell the user to run a command.

segiddins · 2025-03-10T21:54:19Z

schema.rb

rsanheim · 2025-03-15T22:25:31Z

What do you think of skipping conformance tests for jruby and releasing compact index as experimental on jruby for now?

This sounds like a great plan, as I know compact index support would help a lot for our usage of gemstash at Doximity (and we are not on jruby).

Also, getting this merged would possibly nudge jruby/genstash users to debug and fix any follow up issues if needed. 😏

segiddins · 2025-03-16T02:05:28Z

It's passing on jruby now ;)

martinemde

Only small style stuff and calling out boilerplate comments. Looks great.

martinemde · 2025-03-16T02:30:33Z

lib/gemstash/compact_index_builder.rb

+      @auth.check("fetch")
+    end
+
+    # Comment


Review feedback

martinemde · 2025-03-16T02:30:58Z

lib/gemstash/compact_index_builder.rb

+      end
+    end
+
+    # Comment


Additional feedback

martinemde · 2025-03-16T02:31:56Z

lib/gemstash/compact_index_builder.rb

+      fetch_from_storage
+      return result if result
+
+      build_result
+      store_result
+      result


Nit: With a few tweaks and this change you could remove all mentions of @result and the attr_reader :result.

Suggested change

fetch_from_storage

return result if result

build_result

store_result

result

result = fetch_from_storage

return result if result

result = build_result

store_result(result)

result

It also avoids the need to do @result = nil in the rescue for race condition.

martinemde · 2025-03-16T02:37:22Z

lib/gemstash/compact_index_builder.rb

+            @result = file.read
+            resource.save("versions.list" => @result)


...You'd just need to store to a temp var here.

martinemde · 2025-03-16T02:55:51Z

spec/gemstash/compact_index_builder_spec.rb

+      # before
+      Gemstash::GemPusher.new(auth, read_gem("example", "0.1.0")).serve
+      # after


Missing test?

This reverts commit 53d6769.

Signed-off-by: Samuel Giddins <[email protected]>

Remove boilerplate comment placeholders from CompactIndexBuilder classes and delete schema.rb file as requested in review comments.

technicalpickles · 2025-07-09T20:50:43Z

Started work on backfil logic over on #412

indirect approved these changes Jul 9, 2024

View reviewed changes

segiddins force-pushed the segiddins/add-compact-index-support-for-private-sources branch 2 times, most recently from 8fbeaa2 to 8993ad0 Compare July 19, 2024 19:47

segiddins marked this pull request as ready for review July 19, 2024 19:47

olleolleolle reviewed Jul 20, 2024

View reviewed changes

lib/gemstash/compact_index_builder.rb Outdated Show resolved Hide resolved

segiddins force-pushed the segiddins/add-compact-index-support-for-private-sources branch 2 times, most recently from 80b1844 to 0c3e854 Compare July 21, 2024 23:17

segiddins force-pushed the segiddins/add-compact-index-support-for-private-sources branch from 88c97fc to 6bf4489 Compare October 3, 2024 01:08

segiddins force-pushed the segiddins/add-compact-index-support-for-private-sources branch 2 times, most recently from 408a581 to 10aeb06 Compare November 20, 2024 16:10

segiddins force-pushed the segiddins/add-compact-index-support-for-private-sources branch from 10aeb06 to 770c564 Compare February 25, 2025 19:24

segiddins commented Mar 10, 2025

View reviewed changes

deivid-rodriguez mentioned this pull request Mar 11, 2025

Fix bundle lock --add-checksums rubygems/rubygems#8272

Merged

4 tasks

martinemde approved these changes Mar 16, 2025

View reviewed changes

martinemde mentioned this pull request May 17, 2025

Resilience & Security #410

Open

segiddins force-pushed the segiddins/add-compact-index-support-for-private-sources branch from 413a113 to d6dacf3 Compare July 9, 2025 18:57

segiddins added 2 commits July 9, 2025 12:39

Add missing jruby binstub

3c22437

Ensure new enough sqlite3 is used

f5b5800

segiddins added 16 commits July 9, 2025 12:39

Reapply "Add compact index support for private sources"

cedac9c

This reverts commit 53d6769.

Use inner join to simplify query

5437e32

Use Sequel builder methods for complex compact index queries

67a908e

[DNM] Debug sql errors

d7fbc7c

Enable string_agg extension

0c34760

[DNM] Enable error_sql extension

801b4d2

x-plat string_agg

d36a7d2

use concat function for mysql compatibility

1796ac5

Attempt to fix integration specs on postgres on ci

48bce30

Fail early if running old sqlite

d37f758

jruby fixes

203da4b

Signed-off-by: Samuel Giddins <[email protected]>

Fix failing spec on ruby 3.3

07925fa

Signed-off-by: Samuel Giddins <[email protected]>

Bump gem_server_conformance requirement

a46268c

Signed-off-by: Samuel Giddins <[email protected]>

Debug jruby gem server conformance failures

42a33e3

Signed-off-by: Samuel Giddins <[email protected]>

Try requiring bundler/setup on jruby binstub

d0cedb5

Signed-off-by: Samuel Giddins <[email protected]>

Clean up compact index builder and remove schema.rb

2853517

Remove boilerplate comment placeholders from CompactIndexBuilder classes and delete schema.rb file as requested in review comments.

segiddins force-pushed the segiddins/add-compact-index-support-for-private-sources branch from d6dacf3 to 2853517 Compare July 9, 2025 19:40

technicalpickles mentioned this pull request Jul 9, 2025

Start work on backfilling for compact indexes #412

Draft

6 tasks

		@result = file.read
		resource.save("versions.list" => @result)

Uh oh!

Add compact index support for private sources #392

Are you sure you want to change the base?

Add compact index support for private sources #392

Uh oh!

Conversation

segiddins commented Jul 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description:

Tasks:

Uh oh!

indirect left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

technicalpickles commented Jul 31, 2024

Uh oh!

segiddins commented Jul 31, 2024

Uh oh!

technicalpickles commented Jul 31, 2024

Uh oh!

segiddins commented Sep 17, 2024

Uh oh!

martinemde commented Feb 25, 2025

Uh oh!

segiddins commented Feb 25, 2025

Uh oh!

technicalpickles commented Feb 25, 2025

Uh oh!

simi commented Feb 25, 2025

Uh oh!

martinemde commented Feb 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

martinemde commented Mar 6, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

technicalpickles Jul 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rsanheim commented Mar 15, 2025

Uh oh!

segiddins commented Mar 16, 2025

Uh oh!

martinemde left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

technicalpickles commented Jul 9, 2025

Uh oh!

Uh oh!

segiddins commented Jul 9, 2024 •

edited

Loading

martinemde commented Feb 27, 2025 •

edited

Loading

technicalpickles Jul 9, 2025 •

edited

Loading