Conversation
BennyFranciscus
left a comment
There was a problem hiding this comment.
Hey @p8, nice to see Rage getting an entry! Fiber-based non-blocking I/O on Ruby is a cool approach. A few things I spotted:
Missing files:
config/puma.rb— Dockerfile CMD referencespuma -C config/puma.rbbut this file isn't in the PR. Without it, Puma will use defaults which means it won't bind to0.0.0.0:8080. Needs at minimum:
bind 'tcp://0.0.0.0:8080'
threads ENV.fetch('RAILS_MAX_THREADS', 3).to_i, ENV.fetch('RAILS_MAX_THREADS', 3).to_i
workers ENV.fetch('WEB_CONCURRENCY', 0).to_i
preload_app!config/environment.rb—config.rudoesrequire_relative 'config/environment'but there's noconfig/environment.rbin the PR. This will crash on startup.
Missing gems in Gemfile:
sqlite3— controller usesSQLite3::Databasefor the db endpointpuma— used as the app server but not in the Gemfilejson— used forJSON.parse/JSON.generate(might come bundled with Ruby 4.0 stdlib, but explicit is safer)
Potential issue with mattr_accessor:
- That's an ActiveSupport method. Does rage-rb provide it? If not, this will blow up at class load time. Regular
class_attributeor just plain@@varwith accessor methods might be safer.
Compression endpoint:
send_datawithdisposition: :inline— worth checking if Rage supports this. If it's a Rails-specific helper, you might needrender plain: sio.stringwith manual headers instead.
One more thing — no static-h2 in the tests list in meta.json, which is fine, but just confirming that's intentional (no TLS/H2 support in this config).
Otherwise the controller logic looks solid — same patterns as the Rails entry. Looking forward to seeing how Rage compares!
f2e8230 to
b7f5a80
Compare
BennyFranciscus
left a comment
There was a problem hiding this comment.
Nice updates! Looking at the force-pushes, you addressed a bunch of the things I flagged:
- ✅ Switched from puma to
rage -p 8080in the Dockerfile CMD — clean - ✅
config.runow requiresconfig/applicationinstead of the missingconfig/environment - ✅
sqlite3added to Gemfile - ✅
config/application.rbwith proper Rage setup
A couple things still worth checking:
-
mattr_accessorin the controller — does rage-rb provide this? If it comes from ActiveSupport and Rage bundles it, you're good. If not, a simpleclass << self; attr_accessor ... endwould be safer. -
send_datawithdisposition: :inlinein the compression endpoint — same question, does Rage support this method? If not, you could do something like:
response.headers["Content-Type"] = "application/json"
response.headers["Content-Encoding"] = "gzip"
render plain: sio.string-
meta.jsonhas"engine": "puma"but you're now using Rage's built-in server (which is Iodine under the hood iirc). Might want to update that to"iodine"or"rage"to keep it accurate. -
Minor: no
"db"in the tests list in meta.json, but the endpoint is implemented. Intentional?
Getting close though 👍
cda5fd6 to
3080ee8
Compare
BennyFranciscus
left a comment
There was a problem hiding this comment.
Good progress on the refactor! The class-level instance vars with def self. accessors is much cleaner than mattr_accessor.
Spotted one bug though:
Missing accessor for @dataset_items — you define:
def self.db_available = @db_available
def self.large_json_payload = @large_json_payloadBut @dataset_items has no accessor. The json_endpoint action calls self.class.dataset_items which will be nil → always returns 500.
Add:
def self.dataset_items = @dataset_itemsAlso meta.json still has "engine": "puma" — Rage uses Iodine under the hood, so "iodine" or "rage" would be more accurate.
BennyFranciscus
left a comment
There was a problem hiding this comment.
Still seeing the two things from my last review after the latest push:
- Missing
dataset_itemsaccessor — you havedef self.db_availableanddef self.large_json_payloadbut nodef self.dataset_items. Thejson_endpointaction callsself.class.dataset_itemswhich will benil→ instant 500. Just add:
def self.dataset_items = @dataset_itemsmeta.jsonengine — still says"puma". Since you're using Rage's built-in server (Iodine under the hood),"iodine"or"rage"would be more accurate.
Minor things (non-blocking):
"db"not in the tests list in meta.json but the endpoint is implemented — intentional?send_datain the compression endpoint — if that works in Rage, great. If not, therender plain:+ manual headers approach from my earlier comment is a safe fallback.
Getting really close though — just that accessor and the engine field and this should be good to go 🚀
BennyFranciscus
left a comment
There was a problem hiding this comment.
Latest push still has the same two things:
- Missing
dataset_itemsaccessor — line 27 area hasdef self.db_availableanddef self.large_json_payloadbut nodef self.dataset_items. The json endpoint callsself.class.dataset_itemson line 54 → returnsnil→ 500 every time. One-liner fix:
def self.dataset_items = @dataset_itemsmeta.jsonengine — still says"puma". Rage uses Iodine under the hood, so"iodine"or just"rage"would be more accurate.
Both are tiny fixes, happy to approve once they're in 👍
There was a problem hiding this comment.
Nice, the dataset_items accessor is in there now 👍
Just the one remaining thing: meta.json still has "engine": "puma" — since Rage uses Iodine under the hood (not Puma), updating that to "iodine" or "rage" would keep the metadata accurate.
Once the engine field is updated, this is good to go from my side 🚀
BennyFranciscus
left a comment
There was a problem hiding this comment.
LGTM! Both issues resolved — dataset_items accessor is in and meta.json correctly shows "engine": "iodine". Controller logic looks solid, Rage entry is ready to benchmark 🚀
53b515e to
b34a639
Compare
BennyFranciscus
left a comment
There was a problem hiding this comment.
Re-approving after force push — diff still looks great. Clean Rage entry with proper per-request JSON computation, level 1 gzip compression, and fiber-local SQLite connections. Good to go 👍
BennyFranciscus
left a comment
There was a problem hiding this comment.
Clean implementation. Uses Rage's controller API, standard routing, framework-level serialization. jemalloc + YJIT are documented Ruby production settings. Gzip level 1 manually applied — correct. JSON total computed per-request as required. SQLite mmap pragma is standard. No rule violations here. 👍
BennyFranciscus
left a comment
There was a problem hiding this comment.
Re-approving after latest force push — everything still looks good. Rage entry is clean and ready to go 👍
6284740 to
c0f6187
Compare
No description provided.