* [pbs-devel] [PATCH proxmox-backup 0/3] backup client progress log interval
@ 2024-10-21 12:55 Christian Ebner
2024-10-21 12:55 ` [pbs-devel] [PATCH proxmox-backup 1/3] api-types: client: add type to specify " Christian Ebner
` (4 more replies)
0 siblings, 5 replies; 8+ messages in thread
From: Christian Ebner @ 2024-10-21 12:55 UTC (permalink / raw)
To: pbs-devel
These patches allow to specify a time based or size based interval
for progress log output as generated during `proxmox-backup-client
backup` runs.
The client is extended by an optional `progress-log-interval`
parameter, which allows to set the interval as `TimeSpan` or
`HumanByte` parsable input string, depending on the variant prefix.
If set to `none` the progress log output is disabled, if no prefix is
specified, a time based interval is assumed. Lastly, if the parameter
is not given, the default 'time:1m' is used.
Examples for client invocations are:
- no progress logging:
`proxmox-backup-client backup root.pxar:/ --progress-log-interval=none`
- time based progress logging with 1min 30s interval
`proxmox-backup-client backup root.pxar:/ --progress-log-interval="1m 30s"`
`proxmox-backup-client backup root.pxar:/ --progress-log-interval="time:1m 30s"`
- size based progress logging with 512MiB interval
`proxmox-backup-client backup root.pxar:/ --progress-log-interval="size:512MiB"`
Lower limits are set to 1s and 100MiB for the corresponding variant in
order to prevent excessive log output.
Christian Ebner (3):
api-types: client: add type to specify progress log interval
client: progress log: factor out log message generation
client: progress log: allow to specify backup log interval
pbs-api-types/src/client.rs | 73 ++++++++++++++++++++++++
pbs-api-types/src/lib.rs | 3 +
pbs-client/src/backup_writer.rs | 92 +++++++++++++++++++++++++------
proxmox-backup-client/src/main.rs | 27 ++++++++-
4 files changed, 174 insertions(+), 21 deletions(-)
create mode 100644 pbs-api-types/src/client.rs
--
2.39.5
_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
^ permalink raw reply [flat|nested] 8+ messages in thread
* [pbs-devel] [PATCH proxmox-backup 1/3] api-types: client: add type to specify progress log interval
2024-10-21 12:55 [pbs-devel] [PATCH proxmox-backup 0/3] backup client progress log interval Christian Ebner
@ 2024-10-21 12:55 ` Christian Ebner
2024-10-21 12:55 ` [pbs-devel] [PATCH proxmox-backup 2/3] client: progress log: factor out log message generation Christian Ebner
` (3 subsequent siblings)
4 siblings, 0 replies; 8+ messages in thread
From: Christian Ebner @ 2024-10-21 12:55 UTC (permalink / raw)
To: pbs-devel
Implements the `LogInterval` api type, which will be used to specify
the log progress output interval for the proxmox backup client's
backup command.
Currently, 3 variants of progress log output are allowed:
- none
- time based
- size based
Further, implements the methods to parse the string given as API
parameter into the corresponding variant and value.
The time based variant allows to specify an interval given by a
`TimeSpan` compatible value, the size based variant allows to specify
`HumanByte` compatible interval.
If no explicit variant is specified, the time based variant is used.
Possible parameter values are therefore, e.g.:
- `none` to set no interval (disable logging)
- `1m 30s` to set a time based interval, alternatively the variant
might be declared explicitly as `time:1m 30s`.
- `size:512MiB` to set a size based log interval
Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
pbs-api-types/src/client.rs | 73 +++++++++++++++++++++++++++++++++++++
pbs-api-types/src/lib.rs | 3 ++
2 files changed, 76 insertions(+)
create mode 100644 pbs-api-types/src/client.rs
diff --git a/pbs-api-types/src/client.rs b/pbs-api-types/src/client.rs
new file mode 100644
index 000000000..ecc4ad692
--- /dev/null
+++ b/pbs-api-types/src/client.rs
@@ -0,0 +1,73 @@
+use std::str::FromStr;
+
+use anyhow::{bail, Error};
+
+use proxmox_human_byte::HumanByte;
+use proxmox_schema::{const_regex, ApiStringFormat, ApiType, Schema, StringSchema};
+use proxmox_time::TimeSpan;
+
+const_regex! {
+ pub LOG_INTERVAL_REGEX = r"^((none|(size|time):)?[0-9a-z\.\s]*)$";
+}
+
+pub const LOG_INTERVAL_FORMAT: ApiStringFormat = ApiStringFormat::Pattern(&LOG_INTERVAL_REGEX);
+pub const LOG_INTERVAL_SCHEMA: Schema = StringSchema::new("Log interval")
+ .format(&ApiStringFormat::VerifyFn(verify_log_interval))
+ .type_text("(none|[(size|time):]value) (default 'time:60s')")
+ .schema();
+
+#[derive(Clone)]
+pub enum LogInterval {
+ None,
+ Size(HumanByte),
+ Time(TimeSpan),
+}
+
+impl Default for LogInterval {
+ fn default() -> Self {
+ Self::Time(TimeSpan::from_str("60s").unwrap())
+ }
+}
+
+impl ApiType for LogInterval {
+ const API_SCHEMA: Schema = LOG_INTERVAL_SCHEMA;
+}
+
+pub fn verify_log_interval(value: &str) -> Result<(), Error> {
+ let _: LogInterval = value.parse()?;
+ Ok(())
+}
+
+impl FromStr for LogInterval {
+ type Err = Error;
+
+ fn from_str(v: &str) -> Result<Self, Self::Err> {
+ let mut split = v.split(':');
+ let interval = match (split.next(), split.next()) {
+ (Some("none"), None) => LogInterval::None,
+ // Default to time if no explicit variant set
+ (Some(value), None) => LogInterval::Time(TimeSpan::from_str(value)?),
+ (Some(variant), Some(value)) => match variant {
+ "size" => LogInterval::Size(HumanByte::from_str(value)?),
+ "time" => LogInterval::Time(TimeSpan::from_str(value)?),
+ variant => bail!(format!("unexpected log interval variant '{variant}'")),
+ },
+ _ => bail!("expected '(none|[variant:]value)'"),
+ };
+
+ Ok(interval)
+ }
+}
+
+impl std::fmt::Display for LogInterval {
+ fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+ match self {
+ LogInterval::None => write!(f, "none"),
+ LogInterval::Size(value) => write!(f, "size:{value}"),
+ LogInterval::Time(value) => write!(f, "time:{value}"),
+ }
+ }
+}
+
+proxmox_serde::forward_serialize_to_display!(LogInterval);
+proxmox_serde::forward_deserialize_to_from_str!(LogInterval);
diff --git a/pbs-api-types/src/lib.rs b/pbs-api-types/src/lib.rs
index 635292a54..e2eb2f5c6 100644
--- a/pbs-api-types/src/lib.rs
+++ b/pbs-api-types/src/lib.rs
@@ -93,6 +93,9 @@ pub const GROUP_OR_SNAPSHOT_PATH_REGEX_STR: &str =
mod acl;
pub use acl::*;
+mod client;
+pub use client::*;
+
mod datastore;
pub use datastore::*;
--
2.39.5
_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
^ permalink raw reply [flat|nested] 8+ messages in thread
* [pbs-devel] [PATCH proxmox-backup 2/3] client: progress log: factor out log message generation
2024-10-21 12:55 [pbs-devel] [PATCH proxmox-backup 0/3] backup client progress log interval Christian Ebner
2024-10-21 12:55 ` [pbs-devel] [PATCH proxmox-backup 1/3] api-types: client: add type to specify " Christian Ebner
@ 2024-10-21 12:55 ` Christian Ebner
2024-10-21 12:55 ` [pbs-devel] [PATCH proxmox-backup 3/3] client: progress log: allow to specify backup log interval Christian Ebner
` (2 subsequent siblings)
4 siblings, 0 replies; 8+ messages in thread
From: Christian Ebner @ 2024-10-21 12:55 UTC (permalink / raw)
To: pbs-devel
Move the progress log output into a dedicated helper function, so it
can be reused for size based progress log output as well.
Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
pbs-client/src/backup_writer.rs | 21 +++++++++++++++------
1 file changed, 15 insertions(+), 6 deletions(-)
diff --git a/pbs-client/src/backup_writer.rs b/pbs-client/src/backup_writer.rs
index 4d2e8a801..37ee39e2e 100644
--- a/pbs-client/src/backup_writer.rs
+++ b/pbs-client/src/backup_writer.rs
@@ -2,6 +2,7 @@ use std::collections::HashSet;
use std::future::Future;
use std::sync::atomic::{AtomicU64, AtomicUsize, Ordering};
use std::sync::{Arc, Mutex};
+use std::time::Instant;
use anyhow::{bail, format_err, Error};
use futures::future::{self, AbortHandle, Either, FutureExt, TryFutureExt};
@@ -690,12 +691,11 @@ impl BackupWriter {
Some(tokio::spawn(async move {
loop {
tokio::time::sleep(tokio::time::Duration::from_secs(60)).await;
-
- let size = HumanByte::from(stream_len3.load(Ordering::SeqCst));
- let size_uploaded = HumanByte::from(uploaded_len.load(Ordering::SeqCst));
- let elapsed = TimeSpan::from(start_time.elapsed());
-
- log::info!("processed {size} in {elapsed}, uploaded {size_uploaded}");
+ progress_log(
+ stream_len3.load(Ordering::SeqCst),
+ uploaded_len.load(Ordering::SeqCst),
+ &start_time,
+ );
}
}))
} else {
@@ -921,3 +921,12 @@ impl BackupWriter {
Ok(speed)
}
}
+
+#[inline(always)]
+fn progress_log(size: usize, size_uploaded: usize, start_time: &Instant) {
+ let size = HumanByte::from(size);
+ let size_uploaded = HumanByte::from(size_uploaded);
+ let elapsed = TimeSpan::from(start_time.elapsed());
+
+ log::info!("processed {size} in {elapsed}, uploaded {size_uploaded}");
+}
--
2.39.5
_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
^ permalink raw reply [flat|nested] 8+ messages in thread
* [pbs-devel] [PATCH proxmox-backup 3/3] client: progress log: allow to specify backup log interval
2024-10-21 12:55 [pbs-devel] [PATCH proxmox-backup 0/3] backup client progress log interval Christian Ebner
2024-10-21 12:55 ` [pbs-devel] [PATCH proxmox-backup 1/3] api-types: client: add type to specify " Christian Ebner
2024-10-21 12:55 ` [pbs-devel] [PATCH proxmox-backup 2/3] client: progress log: factor out log message generation Christian Ebner
@ 2024-10-21 12:55 ` Christian Ebner
2024-10-21 15:08 ` [pbs-devel] [PATCH proxmox-backup 0/3] backup client progress " Thomas Lamprecht
2024-10-23 9:12 ` Christian Ebner
4 siblings, 0 replies; 8+ messages in thread
From: Christian Ebner @ 2024-10-21 12:55 UTC (permalink / raw)
To: pbs-devel
Adds the optional parameter `progress-log-interval` which allows to
specify the interval to use for backup progress log output.
The progress can be specified as time based value given as a
`TimeSpan` compatible value or as size based progress log, giving a
`HumanByte` compatible value. The variant is switched by the
specified prefix. If no variant prefix is given, the default time
based variant is used.
Minimum values of 1s and 100MiB are set for the corresponding variant
to protect from excessive output.
Further, if the parameter is set to `none` no progress log output is
generated.
Examplary client invocations are:
- no progress logging:
`proxmox-backup-client backup root.pxar:/ --progress-log-interval=none`
- time based progress logging with 1m 30s interval
`proxmox-backup-client backup root.pxar:/ --progress-log-interval="1m 30s"`
`proxmox-backup-client backup root.pxar:/ --progress-log-interval="time:1m 30s"`
- size based progress logging with 512MiB interval
`proxmox-backup-client backup root.pxar:/ --progress-log-interval="size:512MiB"`
Without providing the optional parameter, the current default is set
to 'time:1m'.
Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
pbs-client/src/backup_writer.rs | 81 ++++++++++++++++++++++++-------
proxmox-backup-client/src/main.rs | 27 +++++++++--
2 files changed, 88 insertions(+), 20 deletions(-)
diff --git a/pbs-client/src/backup_writer.rs b/pbs-client/src/backup_writer.rs
index 37ee39e2e..824d4be70 100644
--- a/pbs-client/src/backup_writer.rs
+++ b/pbs-client/src/backup_writer.rs
@@ -12,7 +12,7 @@ use tokio::io::AsyncReadExt;
use tokio::sync::{mpsc, oneshot};
use tokio_stream::wrappers::ReceiverStream;
-use pbs_api_types::{BackupDir, BackupNamespace};
+use pbs_api_types::{BackupDir, BackupNamespace, LogInterval};
use pbs_datastore::data_blob::{ChunkInfo, DataBlob, DataChunkBuilder};
use pbs_datastore::dynamic_index::DynamicIndexReader;
use pbs_datastore::fixed_index::FixedIndexReader;
@@ -53,6 +53,7 @@ pub struct UploadOptions {
pub compress: bool,
pub encrypt: bool,
pub fixed_size: Option<u64>,
+ pub progress_log_interval: Option<LogInterval>,
}
struct UploadStats {
@@ -359,6 +360,7 @@ impl BackupWriter {
options.compress,
injections,
archive,
+ options.progress_log_interval,
)
.await?;
@@ -653,6 +655,7 @@ impl BackupWriter {
compress: bool,
injections: Option<std::sync::mpsc::Receiver<InjectChunks>>,
archive: &str,
+ progress_log_interval: Option<LogInterval>,
) -> impl Future<Output = Result<UploadStats, Error>> {
let total_chunks = Arc::new(AtomicUsize::new(0));
let total_chunks2 = total_chunks.clone();
@@ -671,6 +674,8 @@ impl BackupWriter {
let injected_len = Arc::new(AtomicUsize::new(0));
let injected_len2 = injected_len.clone();
let uploaded_len = Arc::new(AtomicUsize::new(0));
+ let uploaded_len2 = uploaded_len.clone();
+ let previous_byte_fraction = Arc::new(AtomicUsize::new(0));
let append_chunk_path = format!("{}_index", prefix);
let upload_chunk_path = format!("{}_chunk", prefix);
@@ -684,23 +689,34 @@ impl BackupWriter {
let index_csum = Arc::new(Mutex::new(Some(openssl::sha::Sha256::new())));
let index_csum_2 = index_csum.clone();
- let progress_handle = if archive.ends_with(".img")
- || archive.ends_with(".pxar")
- || archive.ends_with(".ppxar")
- {
- Some(tokio::spawn(async move {
- loop {
- tokio::time::sleep(tokio::time::Duration::from_secs(60)).await;
- progress_log(
- stream_len3.load(Ordering::SeqCst),
- uploaded_len.load(Ordering::SeqCst),
- &start_time,
- );
+ let mut progress_handle = None;
+ let mut progress_byte_interval = 0;
+ match progress_log_interval {
+ Some(LogInterval::Time(ref time_span)) => {
+ if archive.ends_with(".img")
+ || archive.ends_with(".pxar")
+ || archive.ends_with(".ppxar")
+ {
+ let duration = std::primitive::f64::from(time_span.clone());
+ progress_handle = Some(tokio::spawn(async move {
+ loop {
+ tokio::time::sleep(tokio::time::Duration::from_secs_f64(duration))
+ .await;
+ progress_log(
+ stream_len3.load(Ordering::SeqCst),
+ uploaded_len.load(Ordering::SeqCst),
+ &start_time,
+ )
+ }
+ }))
}
- }))
- } else {
- None
- };
+ }
+ Some(LogInterval::Size(ref human_byte)) => {
+ progress_byte_interval = human_byte.as_u64() as usize
+ }
+ Some(LogInterval::None) => {}
+ None => {}
+ }
stream
.inject_reused_chunks(injections, stream_len.clone())
@@ -717,6 +733,15 @@ impl BackupWriter {
for chunk in chunks {
let offset =
stream_len.fetch_add(chunk.size() as usize, Ordering::SeqCst) as u64;
+
+ progress_log_by_byte_interval(
+ progress_byte_interval,
+ (offset + chunk.size()) as usize,
+ &previous_byte_fraction,
+ &uploaded_len2,
+ &start_time,
+ );
+
reused_len.fetch_add(chunk.size() as usize, Ordering::SeqCst);
injected_len.fetch_add(chunk.size() as usize, Ordering::SeqCst);
let digest = chunk.digest();
@@ -734,6 +759,14 @@ impl BackupWriter {
total_chunks.fetch_add(1, Ordering::SeqCst);
let offset = stream_len.fetch_add(chunk_len, Ordering::SeqCst) as u64;
+ progress_log_by_byte_interval(
+ progress_byte_interval,
+ offset as usize + chunk_len,
+ &previous_byte_fraction,
+ &uploaded_len2,
+ &start_time,
+ );
+
let mut chunk_builder = DataChunkBuilder::new(data.as_ref()).compress(compress);
if let Some(ref crypt_config) = crypt_config {
@@ -922,6 +955,20 @@ impl BackupWriter {
}
}
+#[inline(always)]
+fn progress_log_by_byte_interval(
+ interval: usize,
+ pos: usize,
+ previous: &Arc<AtomicUsize>,
+ uploaded: &Arc<AtomicUsize>,
+ start_time: &Instant,
+) {
+ if interval > 0 && pos / interval > previous.load(Ordering::SeqCst) {
+ previous.store(pos / interval, Ordering::SeqCst);
+ progress_log(pos, uploaded.load(Ordering::SeqCst), start_time);
+ }
+}
+
#[inline(always)]
fn progress_log(size: usize, size_uploaded: usize, start_time: &Instant) {
let size = HumanByte::from(size);
diff --git a/proxmox-backup-client/src/main.rs b/proxmox-backup-client/src/main.rs
index e4034aa99..bf2ebeded 100644
--- a/proxmox-backup-client/src/main.rs
+++ b/proxmox-backup-client/src/main.rs
@@ -26,9 +26,9 @@ use pxar::accessor::{MaybeReady, ReadAt, ReadAtOperation};
use pbs_api_types::{
Authid, BackupDir, BackupGroup, BackupNamespace, BackupPart, BackupType, ClientRateLimitConfig,
- CryptMode, Fingerprint, GroupListItem, PruneJobOptions, PruneListItem, RateLimitConfig,
- SnapshotListItem, StorageStatus, BACKUP_ID_SCHEMA, BACKUP_NAMESPACE_SCHEMA, BACKUP_TIME_SCHEMA,
- BACKUP_TYPE_SCHEMA,
+ CryptMode, Fingerprint, GroupListItem, LogInterval, PruneJobOptions, PruneListItem,
+ RateLimitConfig, SnapshotListItem, StorageStatus, BACKUP_ID_SCHEMA, BACKUP_NAMESPACE_SCHEMA,
+ BACKUP_TIME_SCHEMA, BACKUP_TYPE_SCHEMA,
};
use pbs_client::catalog_shell::Shell;
use pbs_client::pxar::{ErrorHandler as PxarErrorHandler, MetadataArchiveReader, PxarPrevRef};
@@ -734,6 +734,10 @@ fn spawn_catalog_upload(
optional: true,
default: false,
},
+ "progress-log-interval": {
+ type: LogInterval,
+ optional: true,
+ },
}
}
)]
@@ -746,6 +750,7 @@ async fn create_backup(
dry_run: bool,
skip_e2big_xattr: bool,
limit: ClientRateLimitConfig,
+ progress_log_interval: Option<LogInterval>,
_info: &ApiMethod,
_rpcenv: &mut dyn RpcEnvironment,
) -> Result<Value, Error> {
@@ -781,6 +786,20 @@ async fn create_backup(
let empty = Vec::new();
let exclude_args = param["exclude"].as_array().unwrap_or(&empty);
+ let progress_log_interval = progress_log_interval.unwrap_or_else(|| LogInterval::default());
+ match progress_log_interval {
+ LogInterval::Time(ref time_span) => {
+ if !(std::primitive::f64::from(time_span.clone()) >= 1.0) {
+ bail!("minimum progress log time interval is 1s");
+ }
+ }
+ LogInterval::Size(ref human_byte) => {
+ if !(human_byte.as_u64() >= 100 * 1024 * 1024) {
+ bail!("minimum progress log size interval is 100 MiB");
+ }
+ }
+ LogInterval::None => {}
+ }
let mut pattern_list = Vec::with_capacity(exclude_args.len());
for entry in exclude_args {
@@ -1132,6 +1151,7 @@ async fn create_backup(
previous_manifest: previous_manifest.clone(),
compress: true,
encrypt: crypto.mode == CryptMode::Encrypt,
+ progress_log_interval: Some(progress_log_interval.clone()),
..UploadOptions::default()
};
@@ -1169,6 +1189,7 @@ async fn create_backup(
fixed_size: Some(size),
compress: true,
encrypt: crypto.mode == CryptMode::Encrypt,
+ progress_log_interval: Some(progress_log_interval.clone()),
};
let stats =
--
2.39.5
_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [pbs-devel] [PATCH proxmox-backup 0/3] backup client progress log interval
2024-10-21 12:55 [pbs-devel] [PATCH proxmox-backup 0/3] backup client progress log interval Christian Ebner
` (2 preceding siblings ...)
2024-10-21 12:55 ` [pbs-devel] [PATCH proxmox-backup 3/3] client: progress log: allow to specify backup log interval Christian Ebner
@ 2024-10-21 15:08 ` Thomas Lamprecht
2024-10-21 16:06 ` Christian Ebner
2024-10-23 9:12 ` Christian Ebner
4 siblings, 1 reply; 8+ messages in thread
From: Thomas Lamprecht @ 2024-10-21 15:08 UTC (permalink / raw)
To: Proxmox Backup Server development discussion, Christian Ebner
Am 21/10/2024 um 14:55 schrieb Christian Ebner:
> These patches allow to specify a time based or size based interval
> for progress log output as generated during `proxmox-backup-client
> backup` runs.
>
> The client is extended by an optional `progress-log-interval`
> parameter, which allows to set the interval as `TimeSpan` or
> `HumanByte` parsable input string, depending on the variant prefix.
> If set to `none` the progress log output is disabled, if no prefix is
> specified, a time based interval is assumed. Lastly, if the parameter
> is not given, the default 'time:1m' is used.
>
nice work but I'm a bit torn w.r.t. exposing the variant inside the
value, could be nicer to have it written out in the option name itself,
e.g.:
--progress-interval for time based, as intervals are very often time
based anyway, and it's probably what most user want by default.
--progress-size-interval for size based, albeit the option name is not
really _that_ good, but progress-interval-size, which would be nicer
for choosing between both via autocompletion as they share a longer
prefix, has an even worse ring to it IMO. That's why I'm torn, the
variant as value avoids this odd option naming, but still, multiplexing
option is juts not that great.
What do you think?
Oh, and in above examples I also dropped the "log" as while it certainly
doesn't hurt its IMO also not really required to be able to understand what
it's for.
_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [pbs-devel] [PATCH proxmox-backup 0/3] backup client progress log interval
2024-10-21 15:08 ` [pbs-devel] [PATCH proxmox-backup 0/3] backup client progress " Thomas Lamprecht
@ 2024-10-21 16:06 ` Christian Ebner
2024-10-22 5:35 ` Thomas Lamprecht
0 siblings, 1 reply; 8+ messages in thread
From: Christian Ebner @ 2024-10-21 16:06 UTC (permalink / raw)
To: Thomas Lamprecht, Proxmox Backup Server development discussion
On 10/21/24 17:08, Thomas Lamprecht wrote:
> Am 21/10/2024 um 14:55 schrieb Christian Ebner:
>> These patches allow to specify a time based or size based interval
>> for progress log output as generated during `proxmox-backup-client
>> backup` runs.
>>
>> The client is extended by an optional `progress-log-interval`
>> parameter, which allows to set the interval as `TimeSpan` or
>> `HumanByte` parsable input string, depending on the variant prefix.
>> If set to `none` the progress log output is disabled, if no prefix is
>> specified, a time based interval is assumed. Lastly, if the parameter
>> is not given, the default 'time:1m' is used.
>>
>
> nice work but I'm a bit torn w.r.t. exposing the variant inside the
> value, could be nicer to have it written out in the option name itself,
> e.g.:
>
> --progress-interval for time based, as intervals are very often time
> based anyway, and it's probably what most user want by default.
>
> --progress-size-interval for size based, albeit the option name is not
> really _that_ good, but progress-interval-size, which would be nicer
> for choosing between both via autocompletion as they share a longer
> prefix, has an even worse ring to it IMO. That's why I'm torn, the
> variant as value avoids this odd option naming, but still, multiplexing
> option is juts not that great.
>
> What do you think?
Okay, fine by me as well. To be honest I only followed trough with the
variant in value approach since I did already implemented most of it on
Friday before I did see your email regarding discouraging that approach.
If going for two different optional parameters, one question remains
however: how to handle the no logging case? Should a 0 value indicate
that or do we want another explicit flag for that?
> Oh, and in above examples I also dropped the "log" as while it certainly
> doesn't hurt its IMO also not really required to be able to understand what
> it's for.
Agreed! Thank you for your comments.
_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [pbs-devel] [PATCH proxmox-backup 0/3] backup client progress log interval
2024-10-21 16:06 ` Christian Ebner
@ 2024-10-22 5:35 ` Thomas Lamprecht
0 siblings, 0 replies; 8+ messages in thread
From: Thomas Lamprecht @ 2024-10-22 5:35 UTC (permalink / raw)
To: Proxmox Backup Server development discussion, Christian Ebner
Am 21/10/2024 um 18:06 schrieb Christian Ebner:
> If going for two different optional parameters, one question remains
> however: how to handle the no logging case? Should a 0 value indicate
> that or do we want another explicit flag for that?
Adding new variants is easier than removing existing ones, so for now
I'd go with using a zero interval for that.
If there are requests we can add either a special value, or maybe even
nicer a separate `--no-progress-log` switch, or with a similar name,
like e.g. an alternative could be `--no-progress-interval`.
>> Oh, and in above examples I also dropped the "log" as while it certainly
>> doesn't hurt its IMO also not really required to be able to understand what
>> it's for.
>
> Agreed! Thank you for your comments.
After having written above w.r.t. the disable-switch option I see a bit
more value in having "log" compared to yesterday, but no hard feelings
either way, let's not get hung up on this – you decide.
_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [pbs-devel] [PATCH proxmox-backup 0/3] backup client progress log interval
2024-10-21 12:55 [pbs-devel] [PATCH proxmox-backup 0/3] backup client progress log interval Christian Ebner
` (3 preceding siblings ...)
2024-10-21 15:08 ` [pbs-devel] [PATCH proxmox-backup 0/3] backup client progress " Thomas Lamprecht
@ 2024-10-23 9:12 ` Christian Ebner
4 siblings, 0 replies; 8+ messages in thread
From: Christian Ebner @ 2024-10-23 9:12 UTC (permalink / raw)
To: pbs-devel
superseded-by version 2:
https://lore.proxmox.com/pbs-devel/20241023091103.80792-1-c.ebner@proxmox.com/T/
_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2024-10-23 9:12 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-10-21 12:55 [pbs-devel] [PATCH proxmox-backup 0/3] backup client progress log interval Christian Ebner
2024-10-21 12:55 ` [pbs-devel] [PATCH proxmox-backup 1/3] api-types: client: add type to specify " Christian Ebner
2024-10-21 12:55 ` [pbs-devel] [PATCH proxmox-backup 2/3] client: progress log: factor out log message generation Christian Ebner
2024-10-21 12:55 ` [pbs-devel] [PATCH proxmox-backup 3/3] client: progress log: allow to specify backup log interval Christian Ebner
2024-10-21 15:08 ` [pbs-devel] [PATCH proxmox-backup 0/3] backup client progress " Thomas Lamprecht
2024-10-21 16:06 ` Christian Ebner
2024-10-22 5:35 ` Thomas Lamprecht
2024-10-23 9:12 ` Christian Ebner
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox