Timestamps are in GMT/BST.
[6:44] -leguin.freenode.net- *** Looking up your hostname...
[6:44] -leguin.freenode.net- *** Checking Ident
[6:44] -leguin.freenode.net- *** Found your hostname
[6:44] -leguin.freenode.net- *** No Ident response
[6:44] * DuraLogBot (~PircBot@webster.duraspace.org) has joined #duraspace
[6:44] * Topic is 'Welcome to DuraSpace IRC. This channel is used for formal meetings and is logged - http://irclogs.duraspace.org/'
[6:44] * Set by tdonohue on Thu Sep 15 17:49:38 UTC 2016
[12:26] * mhwood (~mhwood@mhw.ulib.iupui.edu) has joined #duraspace
[13:18] * tdonohue (~tdonohue@dspace/tdonohue) has joined #duraspace
[19:53] <DSpaceSlackBot> <tdonohue> REMINDER: Our DSpace DevMtg starts at the top of the hour here. Agenda: https://wiki.duraspace.org/display/DSPACE/DevMtg+2017-08-02
[20:01] <DSpaceSlackBot> <tdonohue> Hi all, welcome to this week's DSpace DevMtg. Agenda is listed above. Let's do a quick roll call to see who all is "here" today (on either Slack or IRC)
[20:01] <DSpaceSlackBot> <mwood> Hi
[20:01] <DSpaceSlackBot> <terrywbrady> hello
[20:02] <DSpaceSlackBot> <tdonohue> Good, at least a few folks are paying attention. I'll assume others will catch up later ;)
[20:03] <DSpaceSlackBot> <tdonohue> So, first and foremost today...we've stumbled on a series of Hibernate related bugs/issues in DSpace 6.1 that likely warrant an almost immediate 6.2 release
[20:03] <DSpaceSlackBot> Action: sulfrian is here , too
[20:03] <DSpaceSlackBot> <tom_desair> hi
[20:04] <DSpaceSlackBot> <tdonohue> I'd like to go through these tickets and get a quick "latest status", along with an agreement on a 6.2 release plan
[20:04] <DSpaceSlackBot> <tdonohue> The tickets are listed in the agenda (obviously), but we'll start with
[20:04] <DSpaceSlackBot> <tdonohue> https://jira.duraspace.org/browse/DS-3659
[20:05] <DSpaceSlackBot> <tdonohue> PR: https://github.com/DSpace/DSpace/pull/1815
[20:06] <DSpaceSlackBot> <tdonohue> This was extremely serious on demo.dspace.org. It's only reproducible (so far) with RDF enabled, but as it's a bug in the `Context` object, it's possible it affects *other areas of code*
[20:07] <DSpaceSlackBot> <tdonohue> I think this is "fixed" as demo is no longer affected. It seems like a relatively obvious fix (collaboration between @sulfrian , @tom_desair and other)
[20:07] <DSpaceSlackBot> <sulfrian> I think this is only a quickfix, but it should get back the state like 6.0 was working.
[20:08] <DSpaceSlackBot> <mwood> Is demo otherwise behaving normally, still?
[20:08] <DSpaceSlackBot> <tdonohue> Demo is normal except for another separate (but related issue) we'll discuss in a bit. But, yes, this portion of demo is good, and I reran the "full content reset" a few times yesterday to be certain
[20:09] <DSpaceSlackBot> <tdonohue> so, as far as I'm concerned, I think we should merge PR1815 (cherry-pick to master) and close DS-3659, unless others disagree here?
[20:10] <DSpaceSlackBot> <mwood> If nothing else broke on demo when this was applied, and it has been poked enough that we'd likely know if something broke, then I have no objection.
[20:11] <DSpaceSlackBot> <sulfrian> The whole Context reuses one DBConnection stuff will be another issue?
[20:11] <DSpaceSlackBot> <tdonohue> "poked enough" is hard to judge. But, the PR#1815 is essentially a "revert" of an accidental change to an `if` statement between 6.0 and 6.1
[20:11] <DSpaceSlackBot> <tom_desair> It’s restoring the original 6.0 behaviour so I don’t see an immediate problem.
[20:11] <DSpaceSlackBot> <tdonohue> @sulfrian : we'll get to that..it's a larger issue, not in scope for 6.2
[20:12] <DSpaceSlackBot> <tdonohue> Ok, I'll merge this PR, cherry-pick and close 3659.
[20:13] <DSpaceSlackBot> <sulfrian> @tdonohue I am fine with closing DS-3659, if we handle the bigger issue separately.
[20:13] <DSpaceSlackBot> <tdonohue> Next up in our list: https://jira.duraspace.org/browse/DS-3648
[20:13] <DSpaceSlackBot> <tdonohue> This is another Hibernate bug. PR: https://github.com/DSpace/DSpace/pull/1813 And @hpottinger volunteered to test this
[20:13] <DSpaceSlackBot> <tdonohue> @hpottinger have you tested?
[20:14] <DSpaceSlackBot> <hpottinger> working on it now
[20:14] <DSpaceSlackBot> <tdonohue> Oh, I see @hpottinger added a PR comment just a bit ago (at the start of this meeting).
[20:15] <DSpaceSlackBot> <hpottinger> yes... I realized I only had a few items in this test repo, so... heh... I just retried about 8 times
[20:15] <DSpaceSlackBot> <tdonohue> Ok, thanks. Please report back today/early tomorrow if possible...we want to get this in ASAP for a quick 6.2
[20:15] <DSpaceSlackBot> <hpottinger> and on the eight try, it worked
[20:16] <DSpaceSlackBot> <tdonohue> Moving along (in essence of time): https://jira.duraspace.org/browse/DS-3656 / https://github.com/DSpace/DSpace/pull/1812
[20:17] <DSpaceSlackBot> <tdonohue> 3656 is another Hibernate issue, worked on by @tom_desair and @sven
[20:17] <DSpaceSlackBot> <tom_desair> and this is a requirement for PR-1813
[20:17] <DSpaceSlackBot> <sulfrian> The PR 1813 is based on this?
[20:18] <DSpaceSlackBot> <tom_desair> Yes
[20:18] <DSpaceSlackBot> <tdonohue> Oh, I missed that. Ok, so once 1813 is fully tested, this one should be good to go too
[20:18] <DSpaceSlackBot> <tom_desair> Indeed
[20:19] <DSpaceSlackBot> <tdonohue> Ok, so now we get to the last ticket (found today by @sven and @sulfrian and others in dev)
[20:19] <DSpaceSlackBot> <tdonohue> https://jira.duraspace.org/browse/DS-3660
[20:19] <DSpaceSlackBot> <tdonohue> Work in progress PR: https://github.com/DSpace/DSpace/pull/1816
[20:20] <DSpaceSlackBot> <tdonohue> This one is also serious, as if you have "authority" consumer enabled, auto-reindexing won't work
[20:21] <DSpaceSlackBot> <tdonohue> The PR fixes *part* of the issue here, but doesn't work alone (I added one comment to it already)
[20:21] <DSpaceSlackBot> <sulfrian> The fix is not ready yet. I will look into it tomorrow.
[20:21] <DSpaceSlackBot> <tdonohue> @sulfrian : I added one comment to the PR already on a bug I found. The other necessary fix is the refactor of `Context.commit()`
[20:22] <DSpaceSlackBot> <mwood> This also shows a pattern that I think we should be following: when you want to work with a persisted model object, ask the persistence context for it, use it, and forget it. Let the context figure out what to cache.
[20:22] <DSpaceSlackBot> <sulfrian> @tdonohue Yes I already noticed that there could be other objects.
[20:23] <DSpaceSlackBot> <tdonohue> In parallel to this effort, I did refactor `Context.commit()` locally to do a DB commit *prior* to `dispatchEvents()` (this was discussed in great detail in dev Slack today).
[20:23] <DSpaceSlackBot> <sulfrian> But there is a bigger problem. May submission steps call dispatchEvents directly.
[20:24] <DSpaceSlackBot> <tdonohue> I can submit a separate PR for that refactor if we want to treat it separate, but I think it's related to 1816
[20:24] <DSpaceSlackBot> <hpottinger> lots hammering noises coming out of the Context object....
[20:24] <DSpaceSlackBot> <sulfrian> Without calling `Context.commit()`. I do not know, if it is save to replace all `dispatchEvents()` with `commit()`.
[20:24] <DSpaceSlackBot> <sulfrian> But I will try to check this tomorrow.
[20:25] <DSpaceSlackBot> <tdonohue> it probably depends on the usage of dispatchEvents(). If the event is triggered by a change, then commit() likely needs to be called. But, they might need to be looked at individually
[20:25] <DSpaceSlackBot> <sulfrian> Yes, I will do.
[20:26] <DSpaceSlackBot> <mwood> One of the things that commit() does is call dispatchEvents()
[20:26] <DSpaceSlackBot> <tom_desair> In the initial Hibernate implementation, the `commit()` method was removed. ]] a
[20:27] <DSpaceSlackBot> <tdonohue> As I said previously though (in dev), I do want to be careful that we aren't "over fixing" things... i.e. fixing things that look odd but actually are not broken. So, we should be very careful to test any `dispatchEvents()` we replace with `commit()`
[20:27] <DSpaceSlackBot> <tdonohue> But, @sulfrian thanks for volunteering to look at this. I'll gladly help test things out tomorrow
[20:28] <DSpaceSlackBot> <tom_desair> But when fixing another performance issue, it was re-introduced (https://github.com/DSpace/DSpace/commit/b19189634d4d6ca908764880be796513e70aa114#diff-cbd2f003ca152958b516d0e949719ef3R374)
[20:28] <DSpaceSlackBot> <tom_desair> I think that’s the reason you see the call to `dispatchEvents()` and not `commit()`
[20:28] <DSpaceSlackBot> <sulfrian> I already saw a a NPE during the UploadStep because the Event want to access an Item that was not committed before. So this is not only a theoretical problem.
[20:29] <DSpaceSlackBot> <tdonohue> @tom_desair : good to know. That definitely means we should do a global search on `dispatchEvents()` to be sure others shouldn't be changed back to `commit()`
[20:29] <DSpaceSlackBot> <tom_desair> Indeed, that’s something we missed when restoring the `commit()` method.
[20:30] <DSpaceSlackBot> <sulfrian> "fix conflict" is not a very good commit message...
[20:31] <DSpaceSlackBot> <tdonohue> So, to sum up the 6.2 plans. I'd like to get all the issues fixed that we just described (as most revolve around Hibernate and/or `Context`), and then quickly release a 6.2.
[20:31] <DSpaceSlackBot> <tdonohue> Obviously though, if other Hibernate/Context issues are found we can include them. But, I think we should scope 6.2 to fixing these major bugs and get it out ASAP
[20:31] <DSpaceSlackBot> <mwood> Yes, don't hold the door open for anything else unless DSpace is on fire.
[20:32] <DSpaceSlackBot> <tom_desair> Indeed I’m sorry. That was a rebase that did not go very smoothly
[20:32] <DSpaceSlackBot> <tdonohue> If possible, ideally, we release 6.2 this week (or early next). But, it may depend on whether we can close out all these tickets in the next 24-48 hours
[20:33] <DSpaceSlackBot> <tdonohue> That said, is there anyone here (looking at current Committers now) willing to possible help "cut" the 6.2 release?
[20:33] <DSpaceSlackBot> <tom_desair> Ticket that corresponds to that change is https://jira.duraspace.org/browse/DS-3086
[20:34] <DSpaceSlackBot> <mwood> I can help.
[20:34] <DSpaceSlackBot> Action: hpottinger picks up his hat and throws it way up into the stands: I'm traveling, I'd better not volunteer.
[20:34] <DSpaceSlackBot> <tdonohue> Thanks @mwood. I can still help with release notes, etc. I'd just really like to not have to do back to back releases myself ;)
[20:34] <DSpaceSlackBot> <mwood> Yes.
[20:35] <DSpaceSlackBot> <terrywbrady> @mwood , Can I observe the process if that does not delay you? I am out of the office on Friday.
[20:35] <DSpaceSlackBot> <tom_desair> Ticket that corresponds to that change is https://jira.duraspace.org/browse/DS-3086
[20:35] <DSpaceSlackBot> <mwood> Yes.
[20:35] <DSpaceSlackBot> <tdonohue> Ok, so sounds like we have a plan for 6.2 (i.e. putting out the immediate fires). Now, I'd like to move on to the second topic on our Agenda... How DB Connections now work with Hibernate / DSpace 6
[20:36] <DSpaceSlackBot> <hpottinger> oh, that'd be fun, wanna livestream the release of 6.2?
[20:37] <DSpaceSlackBot> <tdonohue> As was "discovered/realized" by several of us at once... DSpace 6 / Hibernate now *shares* DB Connections within a single thread. I.e. two separate `Context` objects may end up sharing the same DB connection if they are in the same "thread"
[20:37] <DSpaceSlackBot> <tdonohue> This is a big change in behavior from DSpace 5.
[20:37] <DSpaceSlackBot> <tdonohue> In DSpace 5, each "Context" established a new DB connection. Context then committed or aborted the connection after it was done (based on results of that request). Context could also be shared between methods if a single transaction needed to perform actions across multiple methods.
[20:37] <DSpaceSlackBot> <tdonohue> In DSpace 6, Hibernate manages the DB connection pool. Each thread grabs a Connection from the pool. This means two Context objects could use the same Connection (if they are in the same thread). In other words, code can no longer assume each new Context() is treated as a new database transaction.
[20:38] <DSpaceSlackBot> <tdonohue> :point_up: Those are the two summaries from the Agenda notes here
[20:39] <DSpaceSlackBot> <sulfrian> I think it is really strange to be able to do `new Context()` and get the current database connection again.
[20:39] <DSpaceSlackBot> <sulfrian> I think, we should either disallow `new Context()` or each Context should get its own connection.
[20:40] <DSpaceSlackBot> <tdonohue> This change in behavior is one of the reasons we see all these recent Context and Hibernate issues... we have old code assuming the "old behavior" and sometimes wrestling with Hibernate
[20:41] <DSpaceSlackBot> <mwood> Exactly. The thing to do, when you don't know what your callers are up to, *was* to grab a new Context and use it, but that isn't true anymore.
[20:42] <DSpaceSlackBot> <tdonohue> At the very least here we all need to be aware of this new behavior (and watch for issues related to it in the code). But, as @sulfrian notes/implies, it's also an opportunity to think about which behavior is "correct" to us (and perhaps either reconfigure Hibernate to support the DSpace 5 way, or fully embrace the new DSpace 6 way)
[20:42] <DSpaceSlackBot> <hpottinger> well... it's not just a new context, it's *any* context you might happen to have, might share a connection with another context.
[20:43] <DSpaceSlackBot> <tdonohue> correction, @hpottinger : Any context *in the same thread* (based on my understanding of the Hibernate configs we have in place). Separate threads should never share a DB connection (again if I'm reading things right)
[20:44] <DSpaceSlackBot> <mwood> The default setup uses a ThreadLocal to keep track of the "current connection".
[20:44] <DSpaceSlackBot> <mwood> There are other ways. Much discussion over in dev yesterday.
[20:45] <DSpaceSlackBot> <sulfrian> We currently explicitly use a singleton service for the dbconnection: https://github.com/DSpace/DSpace/blob/dspace-6_x/dspace-api/src/main/java/org/dspace/core/Context.java#L156
[20:46] <DSpaceSlackBot> <tom_desair> While I agree that multiple `new Context()` object using a single connection is very confusing, the “sharing” feature has the advantage that it is now almost impossible to leak connections from the database pool. This is an issue that still pops up now and then in DSpace 5.
[20:46] <DSpaceSlackBot> <tdonohue> Yes, currently we just let Hibernate's ThreadLocal manage those connections. As I discovered yesterday, we can be more diligent about asking for a *new* DB connection by using `SessionFactory.openSession()` (opens a new session/db connection) vs `SessionFactory.getCurrentSession()` (only opens a new session if one isn't open already)
[20:47] <DSpaceSlackBot> <tdonohue> But we don't want to use `openSession()` too much, as the risk there is we have to then remember to always *close* these new sessions in our code
[20:48] <DSpaceSlackBot> <mwood> If there is 1:1 Session:Context then closing a Context should close the Session.
[20:48] <DSpaceSlackBot> <tom_desair> Yes but sometimes a Context is not closed properly.
[20:49] <DSpaceSlackBot> <tdonohue> @sulfrian : a note on your note about DBConnection being a singleton. That is true, but (perhaps oddly) `DBConnection` (the class) is not the same as a Hibernate Session / DB connection
[20:50] <DSpaceSlackBot> <mwood> DBConnection is poorly named.
[20:50] <DSpaceSlackBot> <tdonohue> Hibernate Sessions/DB connections are managed within `HibernateDBConnection`: https://github.com/DSpace/DSpace/blob/dspace-6_x/dspace-api/src/main/java/org/dspace/core/HibernateDBConnection.java
[20:50] <DSpaceSlackBot> <tom_desair> But I agree with @sulfrian either we - Refactor `new Context()` to something like `Context.getCurrentContext` (and possibly `Context.getNewContext`)
[20:50] <DSpaceSlackBot> <tdonohue> Yes, the `DBConnection` and `HibernateDBConnection` classes both have bad names...they don't actually *represent* a single DB connection...they are just a way to "grab" a database connection via Hibernate
[20:52] <DSpaceSlackBot> <sulfrian> @tdonohue And hibernate db connections are called sessions, right?
[20:52] <DSpaceSlackBot> <tdonohue> @sulfrian : correct. Hibernate db connections = Session
[20:52] <DSpaceSlackBot> <mwood> Somewhere down inside Hibernate it gets connections out of the pool and gives them to sessions.
[20:53] <DSpaceSlackBot> <tdonohue> And `HibernateDBConnection` has the code for how we "grab" / "claim" a new Hibernate Session (connection)
[20:54] <DSpaceSlackBot> <tdonohue> All this said, I do agree with what @tom_desair and @sulfrian have proposed regarding `Context`. I just don't know myself which way is "easier" and/or better (still mulling it over in my head)
[20:54] <DSpaceSlackBot> <sulfrian> `HibernateDBConnection` also has `commit()`. If this is only the interface to get a session, then commit is wrong there.
[20:55] <DSpaceSlackBot> <tdonohue> So, I don't know that there's any "conclusion" to this discussion we can make today. But, perhaps we should start documenting / brainstorming how we might "fix" this, and make the behavior more readily understandable
[20:56] <DSpaceSlackBot> <tdonohue> It's obviously not something we can fix in 6.2. Depending on the scope of the change, maybe we could improve it in 6.3 or 6.4. But, if we are ripping a lot out and starting "fresh", that may need to wait for 7.0
[20:56] <DSpaceSlackBot> <sulfrian> Should this be targeted for 6.3 or 7.0?
[20:56] <DSpaceSlackBot> <sulfrian> Ah
[20:57] <DSpaceSlackBot> <tdonohue> If we are completely changing `Context`, I think that'd need to wait for 7.0. It's a very central class
[20:57] <DSpaceSlackBot> <mwood> I'm concerned that Context has too many jobs, and a lifecycle potentially different from Session.
[20:57] <DSpaceSlackBot> <tdonohue> If we are doing minor refactoring to improve behavior, we could do that in a 6.3 (assuming we test it well, etc)
[20:58] <DSpaceSlackBot> <hpottinger> it's not too early to think about what we'd like for 7.0, though... so... don't let that stop you
[20:58] <DSpaceSlackBot> <tdonohue> But those are just my initial thoughts. If, as we brainstorm, we discover things that *need to change* to make 6.x stable, then we may need to rethink that
[20:59] <DSpaceSlackBot> <tom_desair> Yes there was the idea yesterday of @mwood to split or remove the Context class.
[20:59] <DSpaceSlackBot> <tdonohue> 7.0 is already in development ;) So, yes, this is the time that such work could already start to happen
[20:59] <DSpaceSlackBot> <tom_desair> I that could be a good thing.
[20:59] <DSpaceSlackBot> <mwood> The Way to use something like Hibernate seems to be for a single method to say "gimme a Session", use it, and close it up.
[21:01] <DSpaceSlackBot> <tom_desair> Keeping the Sessions short will also solve a lot of the performance (and memory) issues.
[21:01] <DSpaceSlackBot> <tdonohue> So, I'd just encourage everyone to start brainstorming possible solutions (either for 6.x or for 7.0 or both). If anyone wants to "run with this" with a few ideas, please do feel free to start up a Wiki page to capture thoughts/feedback
[21:01] <DSpaceSlackBot> <tom_desair> (which currently need “hacks” that introduce new bugs)
[21:03] <DSpaceSlackBot> <tdonohue> As we are over time here, I would like to close this up now. I'll keep this on the agenda as an ongoing topic of discussion. But (as my time is really split between 6.x and 7.x), I'd really like to get others (of you) involved in drafting this up, making some suggestions for future discussions
[21:03] <DSpaceSlackBot> <tdonohue> We'll leave it at that for today though, unless there are any final comments/questions?
[21:03] <DSpaceSlackBot> <tom_desair> Then maybe we need to get this on the DSpace 7 agenda ;)
[21:04] <DSpaceSlackBot> <tdonohue> @tom_desair : I think we (DSpace 6 team) are better versed on the problem right now though (as we're seeing this problem manifest itself in maintaining 6.x. Once we have a proposal, then I do think it needs to also get in front of the DSpace 7 team (so they are aware)
[21:04] <DSpaceSlackBot> <terrywbrady> As 6.2 is evaluated, could we include this PR: https://github.com/DSpace/DSpace/pull/1782.
[21:05] <DSpaceSlackBot> <tdonohue> I *will* however mention this issue to the DSpace 7 team.
[21:06] <DSpaceSlackBot> <tdonohue> @terrywbrady : is 1782 ready to go / tested? I've lost track of its status
[21:06] <DSpaceSlackBot> <terrywbrady> I have created so many iterations of this to attempt to satisfy folks... it is ready for testing.
[21:07] <DSpaceSlackBot> <terrywbrady> The schema fix is queued for 7.0
[21:07] <DSpaceSlackBot> <tdonohue> I think the main goal here is 6.2 waits for nothing (but Hibernate / Context issues). I'm perfectly fine with other things getting in that are ready, but might need to lean on others to review other things & merge the stuff that is ready.
[21:08] <DSpaceSlackBot> <tdonohue> So, if 1782 is ready, I'm ok with it. But, I won't have time to verify/test it TBH. If someone else does, awesome
[21:08] <DSpaceSlackBot> <mwood> I agree: if 1782 is ready when the Hibernate stuff is ready, don't delay merging, but if it isn't ready then it should wait for 6.3.
[21:09] <DSpaceSlackBot> <tom_desair> I’ll do my best to look at it tomorrow if no other Hibernate stuff pops up.
[21:09] <DSpaceSlackBot> <tdonohue> Thanks @tom_desair
[21:10] <DSpaceSlackBot> <tdonohue> Otherwise, @terrywbrady please keep pestering folks to test/review 1782 in the coming days
[21:11] <DSpaceSlackBot> <terrywbrady> Will do.
[21:11] <DSpaceSlackBot> <tdonohue> I'm going to close up the official meeting now, as we are 10mins over (and I'll need to run shortly). But, thanks all for the great discussion, and thanks for all the hard work (in dev and here) on the Hibernate / Context issues
[21:11] <DSpaceSlackBot> <mwood> 'bye all.
[21:11] <DSpaceSlackBot> <tom_desair> bye!
[21:11] * mhwood (~mhwood@mhw.ulib.iupui.edu) Quit (Read error: Connection reset by peer)
[21:11] <DSpaceSlackBot> <tdonohue> bye all!
[21:11] <DSpaceSlackBot> <hpottinger> I pinged @kshepherd on #1782
[21:12] <DSpaceSlackBot> <sulfrian> Bye. Need to sleep now. (It's already 11pm here.)
[21:13] * tdonohue (~tdonohue@dspace/tdonohue) has left #duraspace
[22:31] <DSpaceSlackBot> <kshepherd> huh wha?
[22:32] <DSpaceSlackBot> Action: kshepherd goes to find his Testing Hat
These logs were automatically created by DuraLogBot on irc.freenode.net using the Java IRC LogBot.