Skip to content

Address Dictionary query generation#463

Merged
CoreyKaylor merged 5 commits into
JasperFx:masterfrom
baronfel:dictionary_linq
Jul 20, 2016
Merged

Address Dictionary query generation#463
CoreyKaylor merged 5 commits into
JasperFx:masterfrom
baronfel:dictionary_linq

Conversation

@baronfel

@baronfel baronfel commented Jul 19, 2016

Copy link
Copy Markdown
Contributor

Addresses #459

There are some dramatic changes in this.
Applying fixes for the IDictionary<,>.ContainsKey('tkey) and ICollection<KVP<,>>.Contains(KVP<,>) cases was easy enough because those fit into the existing method-call based plugin architecture. There was a wrinkle in that the operator that I want to use in postgres is ?, which is already used by the WhereFragment expansion, so I also took the opportunity to make a version of WhereFragment that has a configurable separator.

The drastic thing was adding in a pluggable IBinaryExpressionParser interface. This was required so that I could easily get at cases where we're indexing into a dictionary on the right or left sides to pull out the keys and values to construct the comparison dictionary. As you'd expect a ton of things broke right off the bat, so I spent some time tweaking the Matches condition on the SimplePropertyBinaryExpressionParser. Please please please please please please sanity check me on that. The tests pass on my local box now but there's no way they're exhaustive in this circumstance.

  • Should we add a publicly-facing extension point for IBinaryExpressionParser instances like there are for IMethodCallParser instances?
  • I'd also like to add some more tests actually storing and querying for data based on these dictionaries before I take the [WIP] off of this.
  • Also, thoughts about generic dictionary access? I think it's safe to say that the most extreme we could go is IDictionary<string, TValue>, because json maps must have string keys, so is that worth going ahead and doing here?
  • Also, I want to buff out the binary parsing some more to accept more than dict["key"] == "value".

@jeremydmiller

Copy link
Copy Markdown
Member

Oh man, how upset w/ me would you be if I asked you to solve the "?" problem a different way? Leave "?" substitution the way that it was, and maybe use custom IWhereFragment's for the places where you need that operator? The question mark thing is a common usage and I'd prefer to keep it.

And would you also mind rebasing this on master?

@baronfel

Copy link
Copy Markdown
Contributor Author

I'm actually in the middle of a rebase (kinda scary looking, with csproj changes :)

I could also do a custom WhereFragment, easy enough to revert that commit. No hurt feelings at all, that's why the [WIP]!

@jeremydmiller

Copy link
Copy Markdown
Member

@baronfel Actually, @CoreyKaylor is in the process of removing all the csproj files and putting us squarely on project.json. You might have a better time waiting for that.

A way we could maybe do this is have the substitution wildcard not be hardcoded inside of WhereFragment. Keep "?" as the default, but let you override that on an individual WhereFragment?

@baronfel

Copy link
Copy Markdown
Contributor Author

Sounds like I'll take a breather then and wait for the csproj churn to finish :D
I can do a separate PR for the configurable WhereFragment as a smaller chunk while I'm waiting for all that to settle.

@CoreyKaylor

Copy link
Copy Markdown
Contributor

Already in master.

@baronfel

Copy link
Copy Markdown
Contributor Author

Thanks, rebasing and making the WhereFragment configurable.

""UserName"": ""{UserName}"",
""Birthdate"": ""{Birthdate.ToString("s")}""
}}".Replace("\r\n", "");
}}".Replace("\r\n", "").Replace("\n", "");

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

needed this to make the tests green up on my machine

@CoreyKaylor

Copy link
Copy Markdown
Contributor

Looks like quite a few failing tests on the build server for this branch.

http://build.fubu-project.org/viewLog.html?buildId=47864&tab=buildResultsDiv&buildTypeId=Marten_Master&guest=1

@baronfel

baronfel commented Jul 20, 2016

Copy link
Copy Markdown
Contributor Author

Oh that's expected. I've been having trouble running all the tests successfully on my machine (my poor docker Postgres instance seems to be getting swamped), but I'm still in the process of cleaning things up on my end. I'll remove the [WIP] and ping you all when things are closer to done, I just wanted initial feedback and validation on the approach.

@CoreyKaylor

Copy link
Copy Markdown
Contributor

Just making sure we weren't the ones dropping the ball on getting this taken in. 😄

@baronfel

baronfel commented Jul 20, 2016

Copy link
Copy Markdown
Contributor Author

I'm going to reel this back a bit and skip doing the dict[key] == value pattern right now, because that's a TON of extra work around making sure that we flatten/resolve the left and/or right expressions correctly to try and pull the value out of them to construct the json dictionary. So just looking at the Contains(KVP) and ContainsKey(K) methods right now.

@baronfel baronfel changed the title [WIP] Address Dictionary query generation Address Dictionary query generation Jul 20, 2016
@baronfel

Copy link
Copy Markdown
Contributor Author

Ok, I removed the [WIP] on this PR. We now handle ICollection<KeyValuePair<string,T>>.Contains(KeyValuePair<string,T>> and IDictionary<string,T>.ContainsKey(string) cases generically, and optionally have the ability to set the NpgsqlDbType on a sql parameter, which was necessary when using the @> operator to check for the presence of a key-value pair in a jsonb object. Separate PRs can address the dictionary index question, because that junk is HAIRY!

@CoreyKaylor
CoreyKaylor merged commit e76bd40 into JasperFx:master Jul 20, 2016
@CoreyKaylor

Copy link
Copy Markdown
Contributor

Thanks

jeremydmiller added a commit that referenced this pull request Jul 7, 2026
#4710, #420/#434/#463, epic #486 WS3) (#4878)

- DocumentStore resolves IEventStore.MaxConcurrentRebuildsPerDatabase:
  explicit StoreOptions.Projections knob wins, else max(1, MaxPoolSize/8)
  derived from the default database's Npgsql pool, else unbounded
- TryCreateUsage populates EventStoreUsage.MaxConcurrentRebuildsPerDatabase
  (#434 companion) for CritterWatch#309's rebuild dispatcher
- Per-tenant rebuild cancellation contract pinned with TCS-gated
  deterministic mid-flight cancellation tests (TenantPartitionedEventsTests)
- rebuilding.md: Capping Rebuild Concurrency + Cancelling a Rebuild sections
- JasperFx 2.23.0 pin

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
jeremydmiller added a commit that referenced this pull request Jul 7, 2026
…4884, epic #486 WS6/WS3)

rebuildload subcommand load-tests N projections x many partitioned
tenants rebuilding concurrently and sweeps the three governor knobs
(MaxConcurrentRebuildsPerDatabase cap, MaxConcurrentEventLoads/
BatchWritesPerDatabase) + EnableExtendedProgressionTracking, measuring
wall-clock, peak/idle connections and mt_event_progression contention
per configuration via the existing ConnectionSampler + ProgressionLock
Sampler. Outer cap enforced by SemaphoreSlim(cap) mirroring
ProjectionHost.RebuildProjectionsWithCapAsync (#463). Prints a
comparison table + advisory recommendation; never changes a shipped
default (measurement only).

Local-scale finding (20 tenants x 8 projections x 1200 events, pool 100
=> default cap 12): peak conns ~= min(cap, projections)+1 (outer cap is
the dominant connection driver, validating the two-layer model); zero
progression waiters at every cap; wall-clock flattens past cap=4;
extended progression tracking within noise. Recommendation: CONFIRM the
max(1, MaxPoolSize/8) cap + governor default of 4. Derivation documented
in rebuilding.md.

--databases > 1 (sharded rebuild sweep) noted as a follow-up; the
governor tuning evidence lives on a single database.

Addresses #4884

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XNfeNz73Q3EdiTgSeDbo96
jeremydmiller added a commit that referenced this pull request Jul 7, 2026
… defaults (#4884, epic #486 WS6/WS3) (#4896)

* daemonload over pooled sharded databases (#4882, epic #486 WS6)

--databases N pools the tenants across N scaletest_dl_shard_* databases
via MultiTenantedWithShardedDatabases (explicit round-robin placement,
one daemon per shard). ShardConnectionSampler groups pg_stat_activity by
datname so the --max-connections gate is enforced PER DATABASE, plus
per-tenant catch-up verification on every shard and a database-affine
placement check (per-tenant sequence in exactly the home shard).

Local-scale result (25 tenants x 2 shards x 2 projections, 60s):
shard peaks 16/11, all 25 tenants caught up, zero placement violations.
Single-database path (--databases 1, the default) unchanged.

Addresses #4882

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XNfeNz73Q3EdiTgSeDbo96

* Multi-node native-HotCold daemonload scenario (#4883, epic #486 WS6)

daemonload-multinode coordinator launches N daemonload-node child
processes (multiple OS processes of this binary = multi-node without
cluster infra), each an IHost running AddAsyncDaemon(HotCold) over one
shared tenant-partitioned store on one DaemonLockId. Coordinator appends
under load, samples pg_stat_activity per node x database (NodeConnectionSampler),
optionally kills the leader (--kill-leader-after-seconds) and verifies a
survivor takes over, then confirms per-tenant catch-up to each tenant's
own ceiling. --max-connections-per-node gates each node's footprint.

MultiNodeStore centralizes the shared config so coordinator and nodes
can't drift. NodeProcess wraps launch/NODE_READY handshake/graceful
stdin-close stop/kill. Reuses DaemonLoadCommand catch-up probe.

Local-scale result (2 nodes, 8 tenants x 2 projections, kill leader at
10s of 30s): leader node0 killed -> node1 took over, 8/8 tenants caught
up, 0 append failures, per-node peak 9-10 connections.

Native HotCold single-DB = one hot leader (per-database lock). Remaining
WS6 multi-node work: cross-node simultaneous distribution over sharded
multi-DB topology + Wolverine-managed mode (wolverine#3328; harness has
no Wolverine dependency). Documented in README.

Addresses #4883

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XNfeNz73Q3EdiTgSeDbo96

* Rebuild-load governor sweep + load-test-informed defaults evidence (#4884, epic #486 WS6/WS3)

rebuildload subcommand load-tests N projections x many partitioned
tenants rebuilding concurrently and sweeps the three governor knobs
(MaxConcurrentRebuildsPerDatabase cap, MaxConcurrentEventLoads/
BatchWritesPerDatabase) + EnableExtendedProgressionTracking, measuring
wall-clock, peak/idle connections and mt_event_progression contention
per configuration via the existing ConnectionSampler + ProgressionLock
Sampler. Outer cap enforced by SemaphoreSlim(cap) mirroring
ProjectionHost.RebuildProjectionsWithCapAsync (#463). Prints a
comparison table + advisory recommendation; never changes a shipped
default (measurement only).

Local-scale finding (20 tenants x 8 projections x 1200 events, pool 100
=> default cap 12): peak conns ~= min(cap, projections)+1 (outer cap is
the dominant connection driver, validating the two-layer model); zero
progression waiters at every cap; wall-clock flattens past cap=4;
extended progression tracking within noise. Recommendation: CONFIRM the
max(1, MaxPoolSize/8) cap + governor default of 4. Derivation documented
in rebuilding.md.

--databases > 1 (sharded rebuild sweep) noted as a follow-up; the
governor tuning evidence lives on a single database.

Addresses #4884

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XNfeNz73Q3EdiTgSeDbo96

---------

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants