[08:47:08] dcausse: did you see ottomata’s comment on the event-utilities’ topic filter: https://gerrit.wikimedia.org/r/c/wikimedia-event-utilities/+/958926/comments/b2873b66_19266196 ? After all it’s only used for when creating kafka topics. Shall move the topic filter to the kafka source factory methods instead? [08:48:16] pfischer: thanks for the heads up, I'll respond [08:50:49] pfischer: do you have a use-case in mind where you'd like to filter on a per-stream basis rather than a per-application basis? [08:56:00] I’ve been going through the code and I’m somewhat torn: The way we use it, it makes sense to configure the filter once for all event streams. After all, EventStream has a notion of topic. Then again, there’s only one source so far (kafka) that uses that filter which would be in favour of keeping it close to the factory method creating that source. [09:04:18] I think it’s alright to leave it as it is right now. Maybe I’d extend the comment for the topicFilter builder method to explain that this is currently only picked up by kafka sources/by any source that has a notion of topics. [09:06:21] I'm no opposed to moving it elsewhere but I feel that we'll have to revisit these kafkaSourceBuilder builder methods, not a big fan of massively overloading a method to expresse different use-cases [10:32:33] lunch [10:41:17] lunch [13:00:34] dcausse: +1 to revisiting, the EventDataStreamFactory thing has become larger than how it started already. I like the intent, but the naming is not quite right which indicates to me that something could be done better here. [13:01:52] could we at least remove the '.' stuff from that code? and just pass in the full prefix when needed, e.g. 'eqiad.' ? been trying to avoid codifying the '.' convention if it isnt' needed to be [13:02:12] eventgate configs 'topic_prefix' setting is not aware of any separators [13:02:25] sure [13:02:56] if you have ideas on how to make this cleaner perhaps best is to continue the discussion around a patch in gerrit? [13:10:55] I didn't have a chance to finish the migration patches, but I'm working on them now [13:12:43] o/ [13:21:38] uggh, it's too early in the morning for my IDE to be messing with me like this ;( [13:24:32] :) [14:48:18] \o [14:48:24] o/ [14:56:32] PR for staging values is ready for review. I'm sure it'll need some tweaking ;) https://gerrit.wikimedia.org/r/967229 [15:11:18] eh, probably hold off on ^^ , still needs some changes to helmfile and base values.yaml [15:13:15] ok :) [15:14:14] perhaps add a "[WIP] " to your commit line [15:19:35] thanks, added WIP [15:39:22] * ebernhardson can now clearly see the Update object is null, still not sure why :P [15:39:42] i guess i gotta track down relevant events and feed them into the test suite [15:47:26] * ebernhardson will, sooner or later, remember to look in codfw topics for events and not eqiad [15:55:41] hmm, actually adding a null update check to the test suite we get plenty of existing tests that don't generate an update object. Am i forgetting something, should it be null sometimes? [15:58:33] looking [15:58:56] when switching to late fetching we might have missed something [15:59:16] dcausse: it looks like we forget to ever call sourceEvent.setUpate in UpdateEventConverters::fromPageChange [16:00:37] might have been lost in the refactor, trying to find what we used to do in git history [16:01:13] * ebernhardson wonders again about how to write something where it's impossible to construct an invalid object...but thats never easy :P [16:01:57] but in my ideal world...invalid objects fail on compilation because there is no way to construct an invalid object [16:03:39] hm... I feel it's normal to not have an "Update" field when you enter the window [16:04:04] yea, it's not like there is really much to say there. The update would amount to a request to fetch [16:04:45] I wonder why the tests still passed tho [16:05:12] perhaps the mistake is to re-use the same object for both pre and post fetch [16:05:43] I noticed that some of the k8s-services use port 9093 for Kafka, but we use port 9092. Guessing it's secure vs insecure port? [16:05:54] inflatador: 9093 is tls, 9092 is plaintext [16:06:00] should prefer tls [16:06:28] inflatador: yes wdqs has to use 9092 still, the code does not pass the proper kafka options [16:06:39] inflatador: and i learned yesterday...kafka used in flink doesn't auto-detect, so if you give it a tls port but it's expecting plaintext, it gives timeouts [16:06:59] np, will keep at 9092 for now [16:08:50] * ebernhardson now wonders why the producer it passes, clearly still something i'm not understanding here :) [16:14:48] integration test doesn't end up invoking the merge [16:19:40] i notice that the UpdateEventSource.augmentWithCirrusDoc no longer does anything, [16:22:02] I just deleted the 'puppet deploy window' invite...if anyone thinks it's useful LMK and I can add it back [16:23:07] looks like fetch is done entirely based on the change type, would provide the Update object, and probably the merger was post-fetch before so update was always available [16:23:31] yes that's my understanding [16:23:37] that means...the augmentWithCirrusDoc should be removed, and merger will need to handle the null update object [16:23:53] sounds reasonable [16:31:21] ebernhardson: shall I look into this? [16:35:17] pfischer: i imagine you're about done for the day? I can probably work this out today [16:36:09] Almost, I have roughly 1 more hour, but could also work on the last comment from David to move my MR forward [16:52:28] * ebernhardson wonders if it would be cleaner to use an empty Update object that represents a fetch request instead of null checks and optional wrapping [16:54:20] dcausse: thank you for your review, I’m not sure I got your concern correctly [16:57:46] ebernhardson: you would always populate the update property? We could do that but that would still require logic to dermine its emptiness. Would it help if getUpdate returned Optionl instead? That would allow for more fluent access. [17:03:30] OK, https://gerrit.wikimedia.org/r/c/operations/deployment-charts/+/967229/ is really ready for review [17:03:42] sorry I goofed that one up last time ;( [17:04:09] pfischer: i suppose it just feels a bit awkward and overspecialized to detect the right conditions in UpdateEventMerger, it allows a null update but only if it represents a fetch request that we indirectly detect by repeating the conditions used in the bypassble fetcher [17:09:21] it's not the end of the world though, we can keep it simple [17:09:52] have something simple written up now, just figuring out shape of stuff to add to IT to ensures it invokes merges [17:15:25] Yeah, that does not feel right, to have such logic spread out/duplicated across multiple places. For the ITs I used a StringDeserializer instead of the default one (byte[]) and parsed the kafka messages with ObjectMapper so I could easily traverse them. This way I was able to check if certain paths where set. [17:20:05] lunch, back in ~40 [17:31:07] Alright, I’m out for today (and tomorrow) [17:57:35] inflatador, ryankemper: I'll skip the pairing session again, sorry. A few things to finish before the vacation [18:13:57] ACK [19:05:10] back in ~90, [19:20:23] Is there any way I could have Commons structured data continuously updated in a Blazegraph server? [19:26:42] hare: hmm, sadly i don't know. Maybe someone will know overnight [19:43:20] * ebernhardson can't seem to find how the devlopment schema for the update_pipeline stream ends up in target/ [19:45:30] * ebernhardson is just blind [20:03:05] hare: sadly I don't think that the RC poller is compatible with commons :( I think we only adapted the updater that runs directly behind kafka (via flink) and thus almost impossible to use outside the WMF infra... [20:05:12] What witchcraft do you suppose it would take to reveal that Kafka stream to, for instance, Cloud Services? [20:09:11] hare: I don't know :(, I think if we could get someone to work on T294133 that would greatly help both wikidata and commons re-users [20:09:12] T294133: Expose rdf-streaming-updater.mutation content through EventStreams - https://phabricator.wikimedia.org/T294133 [20:10:05] I'm super famaliar with EventStreams so not sure how much work is involved here [20:10:09] *not [20:39:53] back [21:09:56] The work involved amounts to taking data from Flink and piping it over to EventStream. Do I have that right? [21:10:34] Or, bypassing Flink, and piping Kafka directly to EventStream [21:11:25] Is the EventStream itself an event listener? Could it listen to Kafka events? (Does Kafka have events?) [21:15:05] Reading the documentation, EventStream *is* built on Kafka. So it seems more like a matter of opening something up within Kafka. Is there a reason you couldn't directly open a new stream? [21:18:17] * inflatador is getting confused between environments and releases in deployment-charts repo [21:20:12] ah, looks like you can do environment- and release-specific overrides [21:20:25] and both...I guess we'll have something like values-wikidata-staging.yaml [21:49:36] https://gerrit.wikimedia.org/r/c/operations/deployment-charts/+/967229 jenkins is unhappy with helmfile.yaml ...trying to figure out what I broe [21:58:07] looking [21:58:26] https://gerrit.wikimedia.org/r/c/operations/deployment-charts/+/967229/13/helmfile.d/services/rdf-streaming-updater/helmfile.yaml ... `in ./helmfile.yaml line 61: did not find expected key` ... any suggestions? [21:58:43] I've tried moving around some of the values stuff, but no luck so far [21:58:44] you're right that thats a useless error message :) [21:59:38] helmfile.d/services/mw-api-ext/helmfile.yaml is the first helmfile I found that has releases other than "main" [21:59:50] yea, cirrus does too [21:59:53] that part is fine [22:00:30] inflatador: i'm not sure you can use the backtick syntax in the environments section [22:00:43] inflatador: i suspect that syntax is only valid inside the templates: section [22:00:54] ebernhardson ACK, that was just a stab...it was failing before that too with same error [22:01:01] the difficulty is this has two layers of templating that happens [22:01:23] oh, hmm [22:01:42] I broke it between patchset 10 and 11 [22:02:36] ebernhardson found it...extra ] 's [22:02:50] towards the bottom of the file [22:03:09] it's amazing how hard those things are to see [22:03:20] need better syntax highlighting that makes them super obvious [22:03:46] IDE is supposed to help, but it already paints helmfile.yaml full of red, so I guess I just tuned it out [22:05:22] i don't think i have great yaml highlighting either, but at least my json highlighting does a good job finding broken stuff [22:05:39] suspect it's because yaml is far too flexible and invalid looks look valid-ish [22:06:50] so you render the yaml as json and then look at it in IDE? [22:14:07] wish that would work :) no, i just look sadly at the yaml and try and detect the problem. No great solutions for that [22:15:05] yeah, I guess it has to render too many things from too many places. The debug trick you taught us is pretty useful though