ISSUE_INDEXER_QUEUE_CONN_STR used as ISSUE_INDEXER_QUEUE_DIR

[nuno-silva]

• Gitea version (or commit ref): 1.13.1 (and also master as of 8688c2b)
• Git version: NA
• Operating system: docker
• Database (use [x]): NA
• Can you reproduce the bug at https://try.gitea.io: NA
• Log gist: NA

Description

It seems ISSUE_INDEXER_QUEUE_CONN_STR is being used as ISSUE_INDEXER_QUEUE_DIR.
Steps to reproduce (from source):

• set this in app.ini (and some sane values for the other ini sections):

[indexer]
ISSUE_INDEXER_TYPE = bleve
ISSUE_INDEXER_PATH = /data/gitea/indexers/issues.bleve

• start gitea web (this one was compiled from source as of 8688c2b):

/data $ GITEA_WORK_DIR=/data/gitea ~/Downloads/gitea/gitea web --config /data/gitea/conf/app.ini

• Notice issues.queue is created, as per the defaults:

/data $ tree gitea/indexers/
gitea/indexers/
├── issues.bleve
│   ├── index_meta.json
│   ├── rupture_meta.json
│   └── store
└── issues.queue
    ├── 000002.ldb
    ├── 000003.log
    ├── CURRENT
    ├── CURRENT.bak
    ├── LOCK
    ├── LOG
    └── MANIFEST-000004

• stop gitea
• rm -r /data/gitea/indexers/
• change the indexer config section to

[indexer]
ISSUE_INDEXER_TYPE = bleve
ISSUE_INDEXER_PATH = /data/gitea/indexers/issues.bleve
ISSUE_INDEXER_QUEUE_CONN_STR = "addrs=127.0.0.1:6379 db=0"

• start gitea web again

• notice that issues.queue does not exist, but a 'addrs=127.0.0.1:6379 db=0' dir was created:

/data $ tree gitea/indexers/
gitea/indexers/
└── issues.bleve
    ├── index_meta.json
    ├── rupture_meta.json
    └── store

1 directory, 3 files
/data $ ls
'addrs=127.0.0.1:6379 db=0'   git   gitea
/data $ ls addrs\=127.0.0.1\:6379\ db\=0/
000001.log  CURRENT  LOCK  LOG  MANIFEST-000000

Notice I did not set ISSUE_INDEXER_QUEUE_TYPE nor ISSUE_INDEXER_QUEUE_DIR anywhere.

---

The config-cheat-sheet does mention that ISSUE_INDEXER_QUEUE_TYPE and ISSUE_INDEXER_QUEUE_DIR are deprecated, but they are present in the example config file:

gitea/custom/conf/app.example.ini

Lines 453 to 460 in 3c96a37

	ISSUE_INDEXER_QUEUE_TYPE = levelqueue
	; When ISSUE_INDEXER_QUEUE_TYPE is levelqueue, this will be the queue will be saved path,
	; default is indexers/issues.queue
	ISSUE_INDEXER_QUEUE_DIR = indexers/issues.queue
	; When `ISSUE_INDEXER_QUEUE_TYPE` is `redis`, this will store the redis connection string.
	ISSUE_INDEXER_QUEUE_CONN_STR = "addrs=127.0.0.1:6379 db=0"
	; Batch queue number, default is 20
	ISSUE_INDEXER_QUEUE_BATCH_NUMBER = 20

---

I was able to reproduce this using:

• gitea 1.13.1, docker, sqlite
• gitea 1.13.1, Gentoo, MySQL (production install)
• master as of 8688c2b), Ubuntu 18, sqlite

Screenshots

NA

[zeripath]

this is intentional.

[nuno-silva]

What's the reasoning behind this being intentional? Shouldn't ISSUE_INDEXER_QUEUE_CONN_STR only do anything at all when ISSUE_INDEXER_QUEUE_TYPE is redis?

[nuno-silva]

CONN_STR in the queue section is also affected (this is from a prod instance running 1.13.1):

[queue]
; Specific queues can be individually configured with [queue.name]. [queue] provides defaults
;
; General queue queue type, currently support: persistable-channel, channel, level, redis, dummy
; default to persistable-channel
TYPE = persistable-channel
; data-dir for storing persistable queues and level queues, individual queues will be named by their type
DATADIR = /var/lib/gitea/queues/
; Default queue length before a channel queue will block
LENGTH = 20 
; Batch size to send for batched queues
BATCH_LENGTH = 20 
; Connection string for redis queues this will store the redis connection string.
CONN_STR = "addrs=127.0.0.1:6379 db=0"

# lsof | grep 6379 | head -2
gitea     16162                      git   22r      REG              254,0      162     918641 /var/lib/gitea/addrs=127.0.0.1:6379 db=0/000010.ldb
gitea     16162                      git   23uW     REG              254,0        0     914026 /var/lib/gitea/addrs=127.0.0.1:6379 db=0/LOCK

...and if I comment out CONN_STR and restart gitea:

# lsof | grep 6379; echo $?
1

Note that it's not hard to end up with CONN_STR defined even if not using redis, since that's how it's done in the example config:

gitea/custom/conf/app.example.ini

Lines 484 to 497 in 3c96a37

	[queue]
	; Specific queues can be individually configured with [queue.name]. [queue] provides defaults
	;
	; General queue queue type, currently support: persistable-channel, channel, level, redis, dummy
	; default to persistable-channel
	TYPE = persistable-channel
	; data-dir for storing persistable queues and level queues, individual queues will be named by their type
	DATADIR = queues/
	; Default queue length before a channel queue will block
	LENGTH = 20
	; Batch size to send for batched queues
	BATCH_LENGTH = 20
	; Connection string for redis queues this will store the redis connection string.
	CONN_STR = "addrs=127.0.0.1:6379 db=0"

So, if this is intentional, it should at least be documented :/

[zeripath]

because it can be used to provide a lot more options to the leveldb queue.

The app.example.ini is NOT supposed to be copied. If you don't know what a setting is you should not set it.

[nuno-silva]

sorry to insist, but...

The app.example.ini is NOT supposed to be copied. If you don't know what a setting is you should not set it.

Fair enough. I can simply not define CONN_STR nor ISSUE_INDEXER_QUEUE_CONN_STR. However, it is not mentioned anywhere in the config-cheat-sheet nor in the example config that the directory settings are overridden by these unrelated "connection" settings. This "intentional behaviour" is totally unexpected and should at least be documented, even if done for legacy/compatibility reasons.

---

because it can be used to provide a lot more options to the leveldb queue.

sure, but why should it override the directory when leveldb is not used?

[zeripath]

are overridden by these unrelated "connection" settings.

They are not unrelated settings at all. Just because you don't know how they relate does not mean that they aren't and aren't that way for a good reason.

sure, but why should it override the directory when leveldb is not used?

How do you think the persistable-channel is persisted? We use a leveldb internally - I'm fairly certain that the docs state that.

---

Anyway let's stop being so aggressive.

The CONN_STR for an underlying level db is a directory or, something of the form:

leveldb://path/to/db?option=value&.... or leveldb:///absolute/path/to/db?option=value&....

with options as per:

gitea/modules/nosql/manager_leveldb.go

Lines 59 to 132 in d989247

	for k, v := range uri.Query() {
	switch replacer.Replace(strings.ToLower(k)) {
	case "blockcachecapacity":
	opts.BlockCacheCapacity, _ = strconv.Atoi(v[0])
	case "blockcacheevictremoved":
	opts.BlockCacheEvictRemoved, _ = strconv.ParseBool(v[0])
	case "blockrestartinterval":
	opts.BlockRestartInterval, _ = strconv.Atoi(v[0])
	case "blocksize":
	opts.BlockSize, _ = strconv.Atoi(v[0])
	case "compactionexpandlimitfactor":
	opts.CompactionExpandLimitFactor, _ = strconv.Atoi(v[0])
	case "compactiongpoverlapsfactor":
	opts.CompactionGPOverlapsFactor, _ = strconv.Atoi(v[0])
	case "compactionl0trigger":
	opts.CompactionL0Trigger, _ = strconv.Atoi(v[0])
	case "compactionsourcelimitfactor":
	opts.CompactionSourceLimitFactor, _ = strconv.Atoi(v[0])
	case "compactiontablesize":
	opts.CompactionTableSize, _ = strconv.Atoi(v[0])
	case "compactiontablesizemultiplier":
	opts.CompactionTableSizeMultiplier, _ = strconv.ParseFloat(v[0], 64)
	case "compactiontablesizemultiplierperlevel":
	for _, val := range v {
	f, _ := strconv.ParseFloat(val, 64)
	opts.CompactionTableSizeMultiplierPerLevel = append(opts.CompactionTableSizeMultiplierPerLevel, f)
	}
	case "compactiontotalsize":
	opts.CompactionTotalSize, _ = strconv.Atoi(v[0])
	case "compactiontotalsizemultiplier":
	opts.CompactionTotalSizeMultiplier, _ = strconv.ParseFloat(v[0], 64)
	case "compactiontotalsizemultiplierperlevel":
	for _, val := range v {
	f, _ := strconv.ParseFloat(val, 64)
	opts.CompactionTotalSizeMultiplierPerLevel = append(opts.CompactionTotalSizeMultiplierPerLevel, f)
	}
	case "compression":
	val, _ := strconv.Atoi(v[0])
	opts.Compression = opt.Compression(val)
	case "disablebufferpool":
	opts.DisableBufferPool, _ = strconv.ParseBool(v[0])
	case "disableblockcache":
	opts.DisableBlockCache, _ = strconv.ParseBool(v[0])
	case "disablecompactionbackoff":
	opts.DisableCompactionBackoff, _ = strconv.ParseBool(v[0])
	case "disablelargebatchtransaction":
	opts.DisableLargeBatchTransaction, _ = strconv.ParseBool(v[0])
	case "errorifexist":
	opts.ErrorIfExist, _ = strconv.ParseBool(v[0])
	case "errorifmissing":
	opts.ErrorIfMissing, _ = strconv.ParseBool(v[0])
	case "iteratorsamplingrate":
	opts.IteratorSamplingRate, _ = strconv.Atoi(v[0])
	case "nosync":
	opts.NoSync, _ = strconv.ParseBool(v[0])
	case "nowritemerge":
	opts.NoWriteMerge, _ = strconv.ParseBool(v[0])
	case "openfilescachecapacity":
	opts.OpenFilesCacheCapacity, _ = strconv.Atoi(v[0])
	case "readonly":
	opts.ReadOnly, _ = strconv.ParseBool(v[0])
	case "strict":
	val, _ := strconv.Atoi(v[0])
	opts.Strict = opt.Strict(val)
	case "writebuffer":
	opts.WriteBuffer, _ = strconv.Atoi(v[0])
	case "writel0pausetrigger":
	opts.WriteL0PauseTrigger, _ = strconv.Atoi(v[0])
	case "writel0slowdowntrigger":
	opts.WriteL0SlowdownTrigger, _ = strconv.Atoi(v[0])
	case "clientname":
	db.name = append(db.name, v[0])
	}
	}

I'm sorry it's incompletely documented but it's the only way to allow any of these options to be set.

---

This "intentional behaviour" is totally unexpected and should at least be documented, even if done for legacy/compatibility reasons.

I don't completely understand how can we be expected that you would have incorrect values in your app.ini. However, feel free to provide a PR improving the documentation.

If you don't understand what is in your app.ini please don't set it. Please go through the rest of your app.ini - I suspect you have a section [log.x] that does nothing amongst lots of other unhelpful logging configuration - https://docs.gitea.io/en-us/logging-configuration may be helpful.

[nuno-silva]

Thanks for clearing that up. It makes a bit more sense now that you explained it.
I'm sorry if I was aggressive.

---

How do you think the persistable-channel is persisted? We use a leveldb internally - I'm fairly certain that the docs state that.

Sorry, I can't find it anywhere :/

Anyway, I think it's clear now that this is a documentation issue.
I'll try to open a PR (probably next week) to improve the docs and the example config to make this a bit more clear.

---

I suspect you have a section [log.x] that does nothing

yep, you're right :/ I'll review my config. Thank you.