Skip to content

make hostnames explicit#617

Closed
whysthatso wants to merge 1 commit intoeikek:masterfrom
whysthatso:patch-1
Closed

make hostnames explicit#617
whysthatso wants to merge 1 commit intoeikek:masterfrom
whysthatso:patch-1

Conversation

@whysthatso
Copy link
Copy Markdown

this should solve #608

this should solve #608
@eikek
Copy link
Copy Markdown
Owner

eikek commented Feb 1, 2021

Hi @whysthatso , thank you a lot! I'm in favor for this change. Still want to ping @bjeanes , because he might have a different view?

I would then also revert the change in docspell.conf, wdyt? (there is only the joex url that has a host-based url)

I think it's nice to be able to simply start new joex containers. But tbh, if it's running on the same machine, one can also just set pool-size to some higher value. So I'd think the common case is covered?

For reference, see PR #552

@bjeanes
Copy link
Copy Markdown
Contributor

bjeanes commented Feb 1, 2021

If you hardcode the hostname in docker compose, then you can ONLY run one of them.

@whysthatso
Copy link
Copy Markdown
Author

If you hardcode the hostname in docker compose, then you can ONLY run one of them.

indeed, as i am under the impression that this would be the standard case, albeit this should be documented.

@bjeanes
Copy link
Copy Markdown
Contributor

bjeanes commented Feb 1, 2021

Still want to ping @bjeanes , because he might have a different view?

I don't mind if this is merged, because I can still do whatever I want in my deployment. But, I think the preferred solution here would be something like tracking "last seen at" for each node and culling nodes that haven't reported in in some period. Could also have it randomly pick a node instead of work down the list (if I correctly understand that this is what is happening).

indeed, as i am under the impression that this would be the standard case, albeit this should be documented.

Perhaps, but being able to elastically scale up workers based on queue size is not possible using just pool-size AIUI, because you'd have to restart the whole joex, which might take time if it's processing a long job.

I would then also revert the change in docspell.conf, wdyt? (there is only the joex url that has a host-based url)

Yes, agreed.

@whysthatso
Copy link
Copy Markdown
Author

i'll try to add something that makes that parameter explicit in the documentation.

@eikek
Copy link
Copy Markdown
Owner

eikek commented Feb 1, 2021

I don't mind if this is merged, because I can still do whatever I want in my deployment.

That's what I figured. People usually have their own deployments and different setups etc and probably use their own compose files (or whatever alternative) anyways.

I think we can't and shouldn't support all possible scenarios. Although I can see that this one seems rather like a "best practice". However, I doubt that people use more than one machine most of the time and with one machine, the pool-size option can be used with one container to have multiple job executors. It is true that you need to restart joex to apply this config change. If people have multi machine setups, they know what they do.

But, I think the preferred solution here would be something like tracking "last seen at" for each node and culling nodes that haven't reported in in some period. Could also have it randomly pick a node instead of work down the list (if I correctly understand that this is what is happening).

Absolutely! This will be fixed. It works as you said; the order of the list is left to the db, so not really random. But I want to notify all nodes, not just one. Currently there is nothing in place to cleanup/watch this list, which is a bug actually. I'm going to create a separate issue for this, was first thinking to use this one, but now I find it better to track separately.

I'd say we merge this change for now, until the bug is fixed. Then we can adopt the docker setup again. -?

@bjeanes
Copy link
Copy Markdown
Contributor

bjeanes commented Feb 1, 2021

That all seems sensible @eikek! Agreed that most people won't use multiple machines and that ON one machine pool-size is sufficient. I'd just add the caveat that pool-size is appropriate for a static size, but unless it auto-reloads config and I don't know that, it isn't as suitable for dynamic situations. Nonetheless, that's an edge case and I think making it work best for the common case (i.e. @whysthatso's case) should take priority.

@eikek
Copy link
Copy Markdown
Owner

eikek commented Feb 2, 2021

Only want to confirm: the config is not auto-reloaded. so pool-size is only appropriate for static sizing. But I like the idea of being able to change pool-size dynamically… might consider this in the future.

@whysthatso Would you mind also adopting docspell.conf? – the joex base-url can now be a constant value. I think this is less confusing when looking at it.

@bjeanes
Copy link
Copy Markdown
Contributor

bjeanes commented Feb 2, 2021

Another reason to change docspell.conf is that without changing it, the app-id will be joex-joex:

app-id = "joex-"${HOSTNAME}

eikek added a commit that referenced this pull request Mar 11, 2021
Reverts #552 to use a simpler
setup. Leave instructions to get back the more advanced setup provided
by PR #522. Also, #617 has more contextual info.

Fixes: #608
@mergify mergify Bot closed this in #702 Mar 11, 2021
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