[FEATURE] Use fsfreeze instead of sync before snapshot

Is your feature request related to a problem? Please describe.
Currently before snapshot operation, we will run a sync to flush the cache into the disk. It will be even safer if we also run fsfreeze against the filesystem, to stop the filesystem from written data to the disk before we take the snapshot. This will ensure the snapshot we took contains the stable filesystem image.
Describe the solution you'd like
Snapshot process:

fsfreeze --freeze <fs_mount_point>, which should include the cache flush. See https://linux.die.net/man/8/fsfreeze
Take a snapshot
fsfreeze --unfreeze <fs_mount_point>

Notice fsfreeze only works for certain filesystems. For the filesystems that doesn't support fsfreeze, sync will be used instead.
Describe alternatives you've considered
A clear and concise description of any alternative solutions or features you've considered.
Additional context
Since we have to keep the freeze period as short as possible, it's better to do it inside the engine. One additional complexity is how can we know where is block device is mounted inside the engine.

ref: https://github.com/longhorn/longhorn/issues/2029#issuecomment-791931309

Verified with Longhorn-master 03/19/2021
Validation - Pass
The test steps from https://github.com/longhorn/longhorn/issues/2187#issuecomment-793703267 works fine.
@joshimoo The snapshot and backup creation worked fine while the writing was in process, but this works fine with v1.1.0 also, how can I make sure that fsfreeze is being validated with these test steps?
Regarding the test cases, we generally do these kind of testing with our pre release testing like basic operations with volume while writing is in the process, but I can't locate exact test case, I'll add this in our pre-release test.

Logs for Froze filesystem
[instance-manager-e-424ff07d] [pvc-43f5d029-511e-48ff-84bd-13545d3a7ef9-e-93cf8583] time="2021-03-19T15:12:59Z" level=info msg="Starting snapshot" snapshot= volume=pvc-43f5d029-511e-48ff-84bd-13545d3a7ef9
[instance-manager-e-424ff07d] [pvc-43f5d029-511e-48ff-84bd-13545d3a7ef9-e-93cf8583] time="2021-03-19T15:13:00Z" level=info msg="Froze filesystem of volume mounted at /var/lib/kubelet/pods/7f3a44c7-fc27-4e08-ac8c-134eb79b3106/volumes/kubernetes.io~csi/pvc-43f5d029-511e-48ff-84bd-13545d3a7ef9/mount" volume=pvc-43f5d029-511e-48ff-84bd-13545d3a7ef9
[instance-manager-r-8b09fc1c] [pvc-43f5d029-511e-48ff-84bd-13545d3a7ef9-r-9320ccbc] time="2021-03-19T15:13:00Z" level=info msg="Replica server starts to snapshot [d77debd9-7670-4cd1-ab7e-eecfd11c4026] volume, user created true, created time 2021-03-19T15:13:00Z, labels map[]"
[instance-manager-r-8b09fc1c] time="2021-03-19T15:13:00Z" level=info msg="Starting to create disk" disk=d77debd9-7670-4cd1-ab7e-eecfd11c4026
[instance-manager-r-8b09fc1c] time="2021-03-19T15:13:00Z" level=info msg="Finished creating disk" disk=d77debd9-7670-4cd1-ab7e-eecfd11c4026
[instance-manager-e-424ff07d] [pvc-43f5d029-511e-48ff-84bd-13545d3a7ef9-e-93cf8583] time="2021-03-19T15:13:00Z" level=info msg="Starting to snapshot: <IP_ADDRESS>:10000 d77debd9-7670-4cd1-ab7e-eecfd11c4026 UserCreated true Created at 2021-03-19T15:13:00Z, Labels map[]"
[instance-manager-r-15523ad4] [pvc-43f5d029-511e-48ff-84bd-13545d3a7ef9-r-290693c4] time="2021-03-19T15:13:00Z" level=info msg="Replica server starts to snapshot [d77debd9-7670-4cd1-ab7e-eecfd11c4026] volume, user created true, created time 2021-03-19T15:13:00Z, labels map[]"
[instance-manager-r-5bdc7ee4] [pvc-43f5d029-511e-48ff-84bd-13545d3a7ef9-r-cab61160] time="2021-03-19T15:13:00Z" level=info msg="Replica server starts to snapshot [d77debd9-7670-4cd1-ab7e-eecfd11c4026] volume, user created true, created time 2021-03-19T15:13:00Z, labels map[]"
[instance-manager-r-5bdc7ee4] time="2021-03-19T15:13:00Z" level=info msg="Starting to create disk" disk=d77debd9-7670-4cd1-ab7e-eecfd11c4026
[instance-manager-r-15523ad4] [pvc-43f5d029-511e-48ff-84bd-13545d3a7ef9-r-290693c4] time="2021-03-19T15:13:00Z" level=info msg="Starting to create disk" disk=d77debd9-7670-4cd1-ab7e-eecfd11c4026
[instance-manager-r-5bdc7ee4] time="2021-03-19T15:13:00Z" level=info msg="Finished creating disk" disk=d77debd9-7670-4cd1-ab7e-eecfd11c4026
[instance-manager-r-15523ad4] [pvc-43f5d029-511e-48ff-84bd-13545d3a7ef9-r-290693c4] time="2021-03-19T15:13:00Z" level=info msg="Finished creating disk" disk=d77debd9-7670-4cd1-ab7e-eecfd11c4026
[instance-manager-e-424ff07d] [pvc-43f5d029-511e-48ff-84bd-13545d3a7ef9-e-93cf8583] time="2021-03-19T15:13:00Z" level=info msg="Starting to snapshot: <IP_ADDRESS>:10000 d77debd9-7670-4cd1-ab7e-eecfd11c4026 UserCreated true Created at 2021-03-19T15:13:00Z, Labels map[]"
[instance-manager-e-424ff07d] time="2021-03-19T15:13:00Z" level=info msg="Starting to snapshot: <IP_ADDRESS>:10000 d77debd9-7670-4cd1-ab7e-eecfd11c4026 UserCreated true Created at 2021-03-19T15:13:00Z, Labels map[]"
[instance-manager-e-424ff07d] time="2021-03-19T15:13:00Z" level=info msg="Finished to snapshot: <IP_ADDRESS>:10000 d77debd9-7670-4cd1-ab7e-eecfd11c4026 UserCreated true Created at 2021-03-19T15:13:00Z, Labels map[]"
[instance-manager-e-424ff07d] time="2021-03-19T15:13:00Z" level=info msg="Finished to snapshot: <IP_ADDRESS>:10000 d77debd9-7670-4cd1-ab7e-eecfd11c4026 UserCreated true Created at 2021-03-19T15:13:00Z, Labels map[]"
[instance-manager-e-424ff07d] [pvc-43f5d029-511e-48ff-84bd-13545d3a7ef9-e-93cf8583] time="2021-03-19T15:13:00Z" level=info msg="Finished to snapshot: <IP_ADDRESS>:10000 d77debd9-7670-4cd1-ab7e-eecfd11c4026 UserCreated true Created at 2021-03-19T15:13:00Z, Labels map[]"
[instance-manager-e-424ff07d] time="2021-03-19T15:13:00Z" level=info msg="Finished snapshot" snapshot= volume=pvc-43f5d029-511e-48ff-84bd-13545d3a7ef9
[instance-manager-e-424ff07d] [pvc-43f5d029-511e-48ff-84bd-13545d3a7ef9-e-93cf8583] time="2021-03-19T15:13:00Z" level=info msg="Unfroze filesystem of volume mounted at /var/lib/kubelet/pods/7f3a44c7-fc27-4e08-ac8c-134eb79b3106/volumes/kubernetes.io~csi/pvc-43f5d029-511e-48ff-84bd-13545d3a7ef9/mount" volume=pvc-43f5d029-511e-48ff-84bd-13545d3a7ef9

@khushboo-rancher Your best bet is the Froze Filesystem log.
You can also add status reporting dd if=/dev/urandom of=/mnt/<volume> conv=fsync status=progress this should force outputting. But all of this is only relevant if the freeze takes longer then a second, which it shouldn't :)

I checked the page after taking the snapshot, it went down after the fsfreeze.

Is this still going to proceed?

@joshimoo @khushboo-rancher
Would you mind providing more detail on the issue caused by the fsfreeze as well as how to reproduce it?
Thank you.

Is this still going to proceed?

@ChipWolf Yes. Any concern or disadvantage of the current sync way?

Is this still going to proceed?

@ChipWolf Yes. Any concern or disadvantage of the current sync way?

Stable snapshots, freezing disk writes

Performance benchmark
The device mapper doesn't introduce significant performance impact.

Longhorn volume with single replica (v1.5.1)

IOPS (Read/Write)
        Random:           15,277 / 9,014
    Sequential:          26,884 / 13,337

Bandwidth in KiB/sec (Read/Write)
        Random:         180,662 / 78,641
    Sequential:         180,682 / 79,799

Latency in ns (Read/Write)
        Random:        388,402 / 371,265
    Sequential:        324,336 / 372,944

Longhorn volume with single replica + device mapper linear device

dmsetup create testdisk --table "0 $(blockdev --getsz /dev/longhorn/testvol) linear /dev/longhorn/testvol 0"

IOPS (Read/Write)
        Random:           15,394 / 9,001
    Sequential:          26,783 / 13,309

Bandwidth in KiB/sec (Read/Write)
        Random:         180,626 / 78,629
    Sequential:         185,223 / 79,812

Latency in ns (Read/Write)
        Random:        385,477 / 369,820
    Sequential:        318,747 / 367,316

cc @shuo-wu @shuo-wu

The issue mentioned in #3125 is due to that fsfreeze freezes the mount point of the given path. Despite the man page stating the path is the pathname of the mount point, it can be any path.
The potential race condition can be addressed by adding a device mapper (dm) device on top of the iSCSI block device. As a result, the block device was exported as a Longhorn volume.
If one wants to take a snapshot of the volume, it can execute dmsetup suspend and the filesystem is automatically frozen (code). In addition to preventing the race condition, the DM device can be used to live migrate a v1 volume to another instance-manager pod as described in #6001 (comment). Its disadvantage is that it cannot be applied to existing volumes.
Any feedback is appreciated. Thank you.
cc @shuo-wu @innobead @james-munson @WebberHuang1118 @Vicente-Cheng

As I know, the suspended DM device should be on the top of the storage stack, according to the architecture, there would be another longhorn device on it?
Since DM introduces an additional layer for processing bio, it could bring performance degradation. However, if we only leverage linear target, maybe it can be ignored.
Aside from above, LGTM, thanks.

If one wants to take a snapshot of the volume, it can execute dmsetup suspend and the filesystem is automatically frozen (code). In addition to preventing the race condition, the DM device can be used to live migrate a v1 volume to another instance-manager pod as described in #6001 (comment). Its disadvantage is that it cannot be applied to existing volumes.

If an existing volume gets reattached, then we are still able to apply this dm device, right?
About live migration, this will be a big benefit for resource usage reduction, because it means there will be no more 2 instance manager pods in the long run if users enable live migration. We can do live migration in a later version, but this version focuses on data consistency.

If one wants to take a snapshot of the volume, it can execute dmsetup suspend and the filesystem is automatically frozen (code). In addition to preventing the race condition, the DM device can be used to live migrate a v1 volume to another instance-manager pod as described in #6001 (comment). Its disadvantage is that it cannot be applied to existing volumes.

If an existing volume gets reattached, then we are still able to apply this dm device, right?

After creating an iSCSI block device /dev/sdx, we will add a dm-linear device on the top of the block device. Then, link the device to /dev/longhorn/<pvc name>. Thus, the block mappings are different from those in the current device implementation. The dm solution cannot be applied to existing volumes.

About live upgrade, this will be a big benefit for resource usage reduction, because it means there will be no more 2 instance manager pods in the long run if users enable live migration. We can do live migration in a later version, but this version focuses on data consistency.
Also, the current implementation using sync is for all filesystems on the host, so it could be expected to have extra performance costs..
After discussing with @derekbit, introducing this extra layer dm device on top of Longhorn volume should be independent from volume type, no matter what type of volume is. It can apply to fs or raw block volume.

As I know, the suspended DM device should be on the top of the storage stack, according to the architecture, there would be another longhorn device on it?

The new storage stack will be like
https://github.com/longhorn/longhorn/issues/2187#issuecomment-1715931212
cc @WebberHuang1118

After creating an iSCSI block device /dev/sdx, we will add a dm-linear device on the top of the block device. Then, link the device to /dev/longhorn/<pvc name>. Thus, the block mappings are different from those in the current device implementation. The dm solution cannot be applied to existing volumes.

Does this mean we need to have two ways to handle existing volume and new volume? If yes, how to differentiate them?

Thanks @derekbit for the detailed explanation.
So briefly, the new design would be like https://github.com/longhorn/longhorn/issues/6001#issuecomment-1653238434, right?
I do not have much concern about this design. It looks like the dm and suspend command would help us to do things like fsfreeze.
Just curious about the client IO and suspend period.
When we do dmsetup suspend, the client IO will stall.
Some sensitive clients will get errors or warnings if this suspension period takes a little bit longer.
Also on https://github.com/longhorn/longhorn/issues/6001#issuecomment-1653238434, we will have the same situation.
TL;DR, I am curious about the trade-off between dm suspension period and client IO.
Others seem good to me! Thanks for it!

Thanks @derekbit for the detailed explanation. So briefly, the new design would be like #6001 (comment), right? (means the /dev/sdx would add a dm on the top of the block device and export it as the longhorn device)
I do not have much concern about this design. It looks like the dm and suspend command would help us to do things like fsfreeze.
Just curious about the client IO and suspend period. When we do dmsetup suspend, the client IO will stall. Some sensitive clients will get errors or warnings if this suspension period takes a little bit longer. Also on #6001 (comment), we will have the same situation.
TL;DR, I am curious about the trade-off between dm suspension period and client IO. Others seem good to me! Thanks for it!

From my understanding,
During DM suspension, the on-the-fly IOs would be kept in the fs layer or dm layer, which depends on the progress of individual IOs. After DM resume is issued, the prisoned IOs will be re-processed.
During the suspension period, the affection depends on the application's implementation. For example, dd has no pre-defined timeout, so there will be no side effects for it. But like @Vicente-Cheng mentioned, it could be a different situation for some sensitive applications.
However, @derekbit only plans to leverage DM suspend to have table reload, since it's an operation with less complexity (compared to snapshot, data migration ... etc.), IMHO, this may not introduce a great suspend period. Thanks.

If one wants to take a snapshot of the volume, it can execute dmsetup suspend and the filesystem is automatically frozen (code). In addition to preventing the race condition, the DM device can be used to live migrate a v1 volume to another instance-manager pod as described in #6001 (comment). Its disadvantage is that it cannot be applied to existing volumes.

If an existing volume gets reattached, then we are still able to apply this dm device, right?

~After creating an iSCSI block device /dev/sdx, we will add a dm-linear device on the top of the block device. Then, link the device to /dev/longhorn/<pvc name>. Thus, the block mappings are different from those in the current device implementation. The dm solution cannot be applied to existing volumes.~

Correct my statement. Just discussed with dm expert @WebberHuang1118 and did a quick test. dm-linear is a simple device and won't change the data layout, so we can detach existing volumes and add the dm-linear device when attaching them.
cc @innobead

This is great! Good cooperation @derekbit @WebberHuang1118

When starting a v1 volume engine, there is a big controller lock for protecting the controller creation, and the WriteAt and ReadAt are also safeguarded by the big lock. Unlike the implementation of v2 volume, we cannot directly add a dm-linear device in startFrontend function. Attempting to do so would result in a deadlock during dm-linear creation. This is due to there are IOs being written to the iSCSI device when creating a dm-linear device.
To address the issue, we have to do

modify startFrontend(): create an iSCSI block device and do not duplicate the iSCSI block device to /dev/longhorn/
introduce exposeFrontend(): add a dm-linear device on the top of the iSCSI block device and duplicate the device /dev/mapper/<volume name> to /dev/longhorn/<volume name>. The function cannot be protected by the controller lock to avoid a dead lock. However, will it introduce any issue?

Before the implementation, it is essential to investigate the necessity of the big controller lock.
Additionally, the dm-linear device introduced also impacts the volume expansion and so on. The scope is much bigger and beyond the ticket.
cc @shuo-wu @innobead

When starting a v1 volume engine, there is a big controller lock for protecting the controller creation, and the WriteAt and ReadAt are also safeguarded by the big lock. Unlike the implementation of v2 volume, we cannot directly add a dm-linear device in startFrontend function. Attempting to do so would result in a deadlock during dm-linear creation. This is due to there are IOs being written to the iSCSI device when creating a dm-linear device.
To address the issue, we have to do

modify startFrontend(): create an iSCSI block device and do not duplicate the iSCSI block device to /dev/longhorn/
introduce exposeFrontend(): add a dm-linear device on the top of the iSCSI block device and duplicate the device /dev/mapper/<volume name> to /dev/longhorn/<volume name>. The function cannot be protected by the controller lock to avoid a dead lock. However, will it introduce any issue?

Additionally, the dm-linear device introduced also impacts the volume expansion and so on. The scope is much bigger and beyond the ticket.
Before the implementation, it is essential to investigate the necessity of the big controller lock.
cc @shuo-wu @Vicente-Cheng @WebberHuang1118 @innobead

Hi @derekbit,
Can you provide your implementation? I'd like to take a loot at it, thanks.

@WebberHuang1118
Not implemented because of the concerns mentioned in https://github.com/longhorn/longhorn/issues/2187#issuecomment-1735473718.

Based on my understanding,
the LH block device is visible after controller.start()->controller.startFrontend()->Tgt.Startup().
Since the block device shouldn't recieve any I/O before LH exposes it, the big controller lock in controller.Start() seems okay to be removed. Is there any concern about holding the lock during controller start?
IIRC, DM device has flush I/Os before table loading, maybe it's the reason @derekbit encountered I/O deadlock.
Thanks.

I'd like my apps to flush to disk if writes are going to be frozen, then the app should be informed when writes resume.
In this proposed implementation will this be possible?
Some context: a few of my apps (by virtue of a proprietary upstream dependency) store some critical data in memory, only periodically flushing data to disk. I know from experience this upstream dependency doesn't handle extended write locks very gracefully, but it has a few methods to suspend/resume writes and flush anything pending in memory to disk.
The volumes backing these apps are generally several hundred GB, sometimes with relatively large deltas between snapshots; they can take a non-trivial amount of time to complete, so I'd be keen to interface with Longhorn somehow to ensure we're keeping that dependency happy.

Status update:
After discussing offline with @innobead and @derekbit, we have decided to pursue the fsfreeze approach (assuming we can prove its safety) for now, with the linear device potentially coming later if it help to improve live engine upgrade for v1 volumes as predicted.
I implemented an idea for making the fsfreeze approach safe in https://github.com/longhorn/longhorn-engine/commit/34f4b2abe5e4e3044f9354ee3ec5325d523f7a8f. It opened file descriptors to all the volume's mount points, then chose one to verify (is it still a mount point) and freeze. The open descriptors prevented anyone (e.g. the CSI driver) from unmounting the file system before the freeze command was issued. @PhanLe1010, @shuo-wu, and I discovered some drawbacks:

The file system can still be lazy unmounted (umount -l) while "locked" in this way. It is unlikely that anyone will lazy unmount the file system in the super brief time period between lock and freeze (since it doesn't appear that the CSI plugin or kubelet do it), but it is a concern.
If the engine crashes mid-snapshot, it cannot unfreeze the file system. Subsequent attempts to unmount it will fail until some component tries to unfreeze it again.

For 1, I am implementing a suggestion from @PhanLe1010 that we use our own mount of the file system instead of a file descriptor on someone else's mount to do the "lock" in https://github.com/longhorn/longhorn-engine/commit/fd512c3288655415e389ed0b515e0fc60e91a327. We control the mount, so there is virtually no danger of us losing it before fsfreeze.
For 2, I am still working on it, but I think it will look something like a suggestion from @shuo-wu. When instance-manager starts up, it will briefly look for mounted Longhorn file systems and attempt to unfreeze them. (If instance-manager was previously down, there is no way these file systems are healthy.) Similarly, when an engine starts up, it will briefly look for mounts of itself and attempt to unfreeze one. We may need to periodically check for mounts of Longhorn file systems for which no engine is running in instance-manager as well, in case there is a scenario in which:

An engine crashes.
Its instance-manager does not crash.
Longhorn never tries to restart the engine.

While I think this feature will be good to implement, I'm a bit worried users will run into corner cases we do not anticipate. I think we should make fsfreeze functionality at least globally configurable (defaulting to on) to give users an out if their environment or use case doesn't play well with it.

Another interesting caveat is that both my method of identifying a file system to freeze and the one implemented in https://github.com/longhorn/longhorn-engine/pull/565/files tend to fall apart for block volumes.
Both implementations look for a file system with the source /dev/longhorn/... as its source. If the container consuming the block volume creates a file system and mounts it to some location:

Its source is /some/path/in/the/container.
The mount is completely invisible to longhorn-engine, since it does not exist in the host mount namespace.

This is probably fine, and maybe even good.

I don't think Kubernetes block volumes are generally used by workloads that create a file system on them and mount them. The Kubernetes docs suggest a more common use case is raw block access for databases.
If a workload IS partitioning a block volume and/or creating file systems on it, it probably assumes it has total control over it. It would be strange to seek out file systems that Longhorn didn't create itself for freezing.

Implementation is currently held up by an ongoing investigation into a manual test failure that can leave the fsfreeze approach AND the dmsetup approach with unkillable workload processes and the need to reboot. Posting the results here as I work through it: https://github.com/longhorn/longhorn/wiki/Freezing-File-Systems-With-dmsetup-suspend-Versus-fsfreeze.

This ticket may need to wait until the question about the test steps are clarified. Thank you.

Just leaving some pointers for validation, we should check following also along with the suggested test case.

Check both backup & snapshot functionality. In addition, recurring jobs.
Let's pay more attention to the all the e2e snapshot and backup related test cases, there should not be new flaky test related to these area.
Run any negative tests related to snapshot/backup ~50 times if any.
Snapshot/backup from previous version and reverting/restoring to them.

Verified pass on longhorn master (longhorn-engine 0402d7, longhorn-instance-manager 90395b)

Performed the test steps successfully without any problems
Reverting the snapshot created in v1.6.2 was successful.
Restoring the backup that created in v1.6.2 was successful.
Loop negative test case Reboot Volume Node While Heavy Writing And Recurring Jobs Exist, Reboot Replica Node While Heavy Writing And Recurring Jobs Exist passed

For regression test related to backup and snapshot, currently there are some failure on master pipeline, I would like to trace the regression results for more days and then close this ticket if there are no errors related to tis ticket.

Close this ticket as verified pass and did not see daily regression failure ralated to this ticket(in build 966, most failures were due to backing image new behavior)


add clarification btw semantic.manifes.json and manifest.json

per slack conv, adding clarificaiton details btw the two files so users understand why both exist.
What are you changing in this pull request and why?

Checklist

[ ] Review the Content style guide so my content adheres to these guidelines.
[ ] For docs versioning, review how to version a whole page and version a block of content.
[ ] Add a checklist item for anything that needs to happen before this PR is merged, such as "needs technical review" or "change base branch."

Adding or removing pages (delete if not applicable):

[ ] Add/remove page in website/sidebars.js
[ ] Provide a unique filename for new pages
[ ] Add an entry for deleted pages in website/static/_redirects
[ ] Run link testing locally with npm run build to update the links that point to deleted pages

adding you here @jtcohen6 in case you have feedback. thank you!


Make Hamming conform to official definition

From issue https://github.com/exercism/exercism.io/issues/1867

Wikipedia says the Hamming distance is not defined for strings of different length.
I am not saying the problems cannot be different, but for such a well-defined concept it would make sense to stick to one definition, especially when the READMEs provide so little information about what is expected from the implementation.

Let's clean this up so that we're using the official definition.

Closes by pull request #42. Thanks to @ChristianWilkie!


Whitelist for slice args that do not need to be mutable

Closes #1005

@alexcrichton all feedback addressed!

Thanks! To avoid the RefCell, could the whitelist be initialized just after a FirstPassRecord is created?

I noticed that we were using Default to initialize them so that felt a little less future proof if it got used in other places.
But for all I know the chances of that are next to zero (don't fully know the flow yet).
So cool I'll follow suit on how we set the builtin_idents https://github.com/chinedufn/wasm-bindgen/blob/ed28ce9db26f78fd06b4bd9c8c11896101b99696/crates/webidl/src/lib.rs#L83

@alexcrichton done - ready for review

Ah yeah we shouldn't be creating too many instances of this struct but if it comes up I'm sure we can deduplicate as necessary!
Thanks!


External Format Contract Refactor

What type of PR is this?
Improvement

What does this PR do / why is it needed ?
External Format Contract Refactor

Which issue(s) this PR fixes:

Fixes #
Other notes for reviewers:
No
Does this PR introduce a user-facing change?

Can we add a description on the key takeaways from the refactor and why we are doing for example SchemaDetail -> Any .
Thanks

Can we add a description on the key takeaways from the refactor and why we are doing for example SchemaDetail -> Any . Thanks

Thanks a lot for pointing this out, have updated this PR will try to update other PR's in future


fix translate failed when enable topn two phase opt

Proposed changes
Issue Number: close #xxx

Further comments
If this is a relatively large or complex change, kick off the discussion at<EMAIL_ADDRESS>by explaining why you chose the solution you did and what alternatives you considered, etc...

(From new machine)TeamCity pipeline, clickbench performance test result:
the sum of best hot time: 44.42 seconds
stream load tsv:          505 seconds loaded<PHONE_NUMBER>9 Bytes, about 141 MB/s
stream load json:         20 seconds loaded<PHONE_NUMBER> Bytes, about 112 MB/s
stream load orc:          64 seconds loaded<PHONE_NUMBER> Bytes, about 16 MB/s
stream load parquet:          31 seconds loaded 861443392 Bytes, about 26 MB/s
insert into select:          29.0 seconds inserted 10000000 Rows, about 344K ops/s
storage size:<PHONE_NUMBER>6 Bytes

run buildall

(From new machine)TeamCity pipeline, clickbench performance test result:
the sum of best hot time: 46.59 seconds
stream load tsv:          508 seconds loaded<PHONE_NUMBER>9 Bytes, about 140 MB/s
stream load json:         20 seconds loaded<PHONE_NUMBER> Bytes, about 112 MB/s
stream load orc:          64 seconds loaded<PHONE_NUMBER> Bytes, about 16 MB/s
stream load parquet:          31 seconds loaded 861443392 Bytes, about 26 MB/s
insert into select:          29.0 seconds inserted 10000000 Rows, about 344K ops/s
storage size:<PHONE_NUMBER>3 Bytes

run buildall

(From new machine)TeamCity pipeline, clickbench performance test result:
the sum of best hot time: 46.09 seconds
stream load tsv:          512 seconds loaded<PHONE_NUMBER>9 Bytes, about 139 MB/s
stream load json:         19 seconds loaded<PHONE_NUMBER> Bytes, about 118 MB/s
stream load orc:          64 seconds loaded<PHONE_NUMBER> Bytes, about 16 MB/s
stream load parquet:          30 seconds loaded 861443392 Bytes, about 27 MB/s
insert into select:          29.3 seconds inserted 10000000 Rows, about 341K ops/s
storage size:<PHONE_NUMBER>1 Bytes


Datasets for Chapter 1 not available from the link

Hi @ageron ,
Thanks for the link to the Jupyter notebooks. The link to the GDP Dataset from IMF appears to not work (for the downloads alone). Would it be possible to upload the CSV files itself to the repo ?
Thanks

Thanks for the information, I'll looking into this today.

That's odd, the link works fine for me: http://goo.gl/j1MSKe (it redirects here and I could download the data without a problem).  In any case the CSVs are available in the datasets/lifesat directory.
Hope this helps.

@ageron Thanks! Much appreciated. On chrome the file got downloaded with an .aspx extension (but the content was tab separated). Appreciate the quick response. I missed that datasets link on the github page. Closing issue.

Cool, thanks for your feedback.  Cheers.


Nothing Populating

Preflight Checklist

[X] I have installed the latest version of Storage Explorer.
[X] I have checked existing resources, including the troubleshooting guide and the release notes.
[X] I have searched for similar issues.

Storage Explorer Version
1.20.1
Regression From
No response
Architecture
x64
Storage Explorer Build Number
20210720.10
Platform
Windows
OS Version
Windows 10 Pro 20H2
Bug Description
When I open Storage Explorer nothing is populated in either the account view or explorer view. To get my account to show up I have to click View --> Account Management.
Steps to Reproduce

Launch Explorer

Actual Experience

Expected Experience
I expected my accounts to be there in account view and storage accounts in explorer view.
Additional Context
This was working on this machine for a long time. Not sure what caused it to break. I have uninstalled\reinstalled several times, removed my account and added back in, and the behavior persists. I have another machine with storage explorer installed and I am able to use it with no issue.
At one point I was using storage explorer in the Azure Portal. I noticed a button that said open in explorer. I clicked that and it launched explorer locally and opened the folder. A message popped up saying the account had been hidden but did not give me any clue as to how that is resolved.

@sql-chris-albert hi! Can you do the following for us:

While Storage Explorer is open, go to Help -> Toggle Developer tools, and then in the panel/window that opens look for console. Click on console, and then let us know if there's any error/red text
Grab us the Storage Explorer logs. When doing this, please set that STG_EX_LOG_LEVEL environment variable to debug. You'll of course need to run storage explorer/reproduce the issue again after setting the environment variable.

Hi Chris, I am facing the same issue, with all the details you described, right after I have upgraded to v1.20.1. I removed the app and downloaded it again, same problem.  By any chance, did you take this up to Microsoft? I would like to upvote there if you did.

Hi Chris, I am facing the same issue, with all the details you described, right after I have upgraded to v1.20.1. I removed the app and downloaded it again, same problem. By any chance, did you take this up to Microsoft? I would like to upvote there if you did.

FYI - I have uninstalled v1.20.1 and installed v.1.19.1 and it works fine.

@alexandru-pet Thanks for the tip. I installed 1.19.1 and everything is now working. Hopefully MS will be able to resolve the issue with the new version.

@alexandru-pet @sql-chris-albert
Albert's logs did not reveal anything to us about what is potentially going wrong. I've created a private build with additional logging. If y'all could download it, run it, gather the storage explorer logs (make sure to set that logs environment variable) and share the logs with us that'd be a big help.
I have two options for running the build:

Standalone zip: https://storageexplorerpublish.blob.core.windows.net/privatebuilds/4768-1/StorageExplorer-win32-ia32.zip
Normal installer: https://storageexplorerpublish.blob.core.windows.net/privatebuilds/4768-1/StorageExplorer-ia32.exe

Since y'all have downgraded to 1.19.1 for now, then you can use the zip to run the private build without losing your current install. Just download the zip, unzip, and then run the .exe in the root folder.
Thanks!

@MRayermannMSFT I downloaded the Zip and unzipped. When I execute the .exe it show a blank window for a second and then nothing. Have tried a few times.

@sql-chris-albert that's weird....I guess if you have time then to do the full install that would be appreciated. Unfortunately you will have to reinstall 1.19.1 afterwards. ☹

2021-08-17_152227_22792.log
2021-08-17_152227_main.log
2021-08-17_152227_renderer.log
2021-08-17_152227_se-markdown-panel_9444.log

Awesome thanks! I can already see that something is "off" compared to what I see on my machine. I'll digest this and get back to you. Thank you thank you thank you a million times. :)

Ok I think those logs have helped me narrow down the area the problem is occurring in. I have a new private build with yet more logging:

Standalone zip: https://storageexplorerpublish.blob.core.windows.net/privatebuilds/4768-2/StorageExplorer-win32-ia32.zip
Normal installer: https://storageexplorerpublish.blob.core.windows.net/privatebuilds/4768-2/StorageExplorer-ia32.exe

2021-08-18_091855_996.log
2021-08-18_091855_main.log
2021-08-18_091855_renderer.log
2021-08-18_091855_se-markdown-panel_23292.log

Sorry it took me a while to get back to you @sql-chris-albert ...
So the latest round of logs give more insight into where Storage Explorer is not working right but I still do not have a lot of insight into why. And on top of that, the where that I have identified is one of the more opaque/black-box parts of our product.
So with all of that in mind I'd like to try something different today. This will be a bit more involved on your part, but if you have the time for it that would be helpful.
I've prepared a couple of private builds. These builds are from commits (aka changes) we made while developing version 1.20. I'd like to use these private builds to help us narrow down when the breaking change was made to Storage Explorer. I've looked through our commits for 1.20, and these ones are my current best guesses for what could be inadvertently causing this problem. What you need to do is simply install the build and find the oldest one with which this issue reproduces on your machine. I do not need you to get me any log files.
So here are the builds:

https://storageexplorerpublish.blob.core.windows.net/privatebuilds/4768-dev-builds/StorageExplorer-ia32-c6194393.exe
https://storageexplorerpublish.blob.core.windows.net/privatebuilds/4768-dev-builds/StorageExplorer-ia32-c4a2872b.exe
https://storageexplorerpublish.blob.core.windows.net/privatebuilds/4768-dev-builds/StorageExplorer-ia32-715c7ea5.exe
https://storageexplorerpublish.blob.core.windows.net/privatebuilds/4768-dev-builds/StorageExplorer-ia32-c5d38b93.exe
https://storageexplorerpublish.blob.core.windows.net/privatebuilds/4768-dev-builds/StorageExplorer-ia32-d2121ada.exe

They are ordered oldest to newest. I suggest you trying them out in either of the following orders:

Try the middle one first (715c7ea5), and if the issue does repro, then work your way backwards from there, or if it doesn't repro, then work your way forwards from there.
Oldest to newest.

Let me know if you have any questions.
Also, @alexandru-pet if you are still around feel free to partake in this as well.

I started in the middle with 715c7ea5, and it worked fine. c5d38b93 worked fine as well. d2121ada is where the issue came back up.

@sql-chris-albert super! I will delve deeper into our changes from that time period!

Hey @sql-chris-albert (and @alexandru-pet), I have a new set of builds to try. Apologies to ask you to keep doing this, but since we cannot repro the issue and logging did not reveal the issue this seems to be the best course of action.
Right now I feel as if d2121ada is what caused the issue. Out of all the possible changes from c5d38b93 to d2121ada (inclusive) it is the one that comes the closest to touching what is failing to finish. So to verify that, I have a build of the commit before d2121ada, along with a couple of others that occurred  after c5d38b93 but before d2121ada that are also suspect. Here they are:

https://storageexplorerpublish.blob.core.windows.net/privatebuilds/4768-dev-builds-2/StorageExplorer-ia32-bcce041d.exe
https://storageexplorerpublish.blob.core.windows.net/privatebuilds/4768-dev-builds-2/StorageExplorer-ia32-b0eed2b6.exe
https://storageexplorerpublish.blob.core.windows.net/privatebuilds/4768-dev-builds-2/StorageExplorer-ia32-e08eb44f.exe
https://storageexplorerpublish.blob.core.windows.net/privatebuilds/4768-dev-builds-2/StorageExplorer-ia32-2c2efa25.exe

The 2c2efa25 exe is the commit directly before d2121ada, so I guess I'd recommend:

Try 2c2efa25, if issue doesn't repro then you're done/we've found the breaking change
If issues does repro then try b0eed2b6
If issue does repro then try  bcce041d or if it doesn't repro then try e08eb44f

Also, if you're curious, we're working on a range of changes from May 12th to June 22nd. So there's a lot of commits to sift through. I'm glad that we were able to so greatly reduce it last night though! The original range of changes was April 15th to June 22nd. 😬
Finally, if my guess about d2121ada is right then we'll have to do some experiments to figure out why the heck that change is breaking the tree view on your machine. The change was just the removing of sending telemetry in an error handler which makes me feel like 🤷♂.

Hi guys, sorry for not being able to join you on this investigation so far. I had some work related tasks for which I was highly dependant on the logs and couldn't risk losing access to them even for an hour. Today I am off early, but I will try to join from Monday.

Cool thanks for finding the time @alexandru-pet!

Tried 2c2efa25 and it did not work.
Tried b0eed2b6 and it did not work.
Tried bcce041d and it did not work.

Ok  @sql-chris-albert & @alexandru-pet, one more build to try:
https://storageexplorerpublish.blob.core.windows.net/privatebuilds/4768-dev-builds/StorageExplorer-ia32-41178ff2.exe
With regards to Chris' findings:

So my guess about d2121ada being the cause was wrong.
Commit bcce041d happened shortly after c5d38b93, and this latest build is right before bcce041d.

Some additional questions (I have the answers to these from the log files but I want to cross check them with y'all):

You have not enabled proxy settings in Storage Explorer. Correct?
Storage Explorer is running in English. Correct?

41178ff2 worked.
I have not enabled proxy settings and my language is set to use system settings which is English.

Ok, I have a new build to try, and now we're back to sharing logs! I've decided to ignore the I can't fathom how that change broke the tree view init'ing and have added logging in the area of the change and also in additional areas that I didn't have before.
@sql-chris-albert & @alexandru-pet, when y'all have some free time please install, generate logs, gather logs, and then share logs from this build:
https://storageexplorerpublish.blob.core.windows.net/privatebuilds/4768-3/StorageExplorer-ia32.exe
Thanks.

2021-08-24_095030_20156.log
2021-08-24_095030_main.log
2021-08-24_095030_renderer.log
2021-08-24_095030_se-markdown-panel_22356.log

@sql-chris-albert & @alexandru-pet
Ok. Still eluding us. I've gone crazy on today's build and have some additional things to gather/ask. First here's the build: https://storageexplorerpublish.blob.core.windows.net/privatebuilds/4768-4/StorageExplorer-ia32.exe
So, for this build:

When you run it, let it sit for at least 30 seconds after the main window appears. I've added some "retry/give up waiting for things to say they are ready" logic to try and generate some actual errors (so far we've seen no errors in any of the logging we've added 😢).
After letting it sit:

As normal, gather the log files, and then share them here
Open the dev tools with F12/via the help menu, go to the console tab, and filter by CloudExplorer.App. and then share what you find. Here's an example of what I mean:

Also some additional questions:

Are you reproducing this on any other machines?
For all machines you are reproducing on, in what ways are those machines similar? For example:

Are they all managed by your companies IT?
Are they running any anti virus software other than Windows Defender?
Do you have any problems with other applications on these machines? For example: Microsoft Teams, VS Code, Slack, Discord, Postman, GitKraken?

I'm out of the office tomorrow. Will get this done on Thursday.

@MRayermannMSFT Any luck with the last round of logs?

@sql-chris-albert sorry, been busy getting ready to ship 1.21.0. No, the new logs have not been anymore helpful. The tree view is proving to be a black hole from which no information escapes...

Is the Windows user you are using the same one that installed Storage Explorer?
Can you try running as admin?
Can you try changing theme after the app has launched?
Can you try using the default or another theme?
Can you go into the F12 tools, go to Elements ctrl+f for cloud-explorer-panel, find the first result and then...

Take a screenshot of that first result (similar to mine below, but leave out the context menu)
Right click on the <html line, choose "Copy element", save that to a text file, and then attach that file here (see screenshot to see what I'm talking about)

Attach this file here as well: C:/Users/calbert/AppData/Local/Programs/Microsoft Azure Storage Explorer/resources/app/out/CloudHub/html/CloudExplorer/index.html, make sure your user account is able to open and read this file as well.

This issue might be caused by us loading modules take longer than expected and timeout. For all of you who run into this problem, could you please try modifying a source file to override the default timeout parameter and see if mitigates the problem?
The file to modify:

Windows:
macOS: /Applications/Microsoft Azure Storage Explorer.app/Contents/Resources/app/out/CloudHub/js/CloudExplorer/config.js

Somewhere at the end of config.js, add a comma and add one line below. Basically we are going to add a waitSeconds parameters to the require object on window. This would extend the timeout period to 30 seconds instead of the default 7 seconds.
Before:
window.require = {
    baseUrl: "../../",
    paths: {
        "ccs": "internal/ccs/ccs-0.1.0",
        "es6-promise": "lib/es6-promise/es6-promise",
        "jquery": "lib/jquery/dist/jquery",
        "knockout": "lib/knockout/dist/knockout",
        "knockout.mapping": "lib/knockout-mapping/knockout.mapping",
        "underscore": "lib/underscore/underscore",
        "underscore.string": "lib/underscore.string/dist/underscore.string",
        "URIjs": "lib/uri.js/src",
        "ActivityLog": "js/ActivityLog",
        "CloudExplorer": "js/CloudExplorer",
        "Common": "js/Common",
        "Providers": "js/Providers",
        "StorageExplorer": "js/StorageExplorer",
        "text": "lib/text/text"
    },
    shim: {
        jquery: {
            exports: "$"
        },
        knockout: {
            deps: ["jquery"],
            exports: "ko"
        },
        "knockout.mapping": {
            deps: ["jquery", "knockout"],
            exports: "mapping"
        }
    }
};

After:
window.require = {
    baseUrl: "../../",
    paths: {
        "ccs": "internal/ccs/ccs-0.1.0",
        "es6-promise": "lib/es6-promise/es6-promise",
        "jquery": "lib/jquery/dist/jquery",
        "knockout": "lib/knockout/dist/knockout",
        "knockout.mapping": "lib/knockout-mapping/knockout.mapping",
        "underscore": "lib/underscore/underscore",
        "underscore.string": "lib/underscore.string/dist/underscore.string",
        "URIjs": "lib/uri.js/src",
        "ActivityLog": "js/ActivityLog",
        "CloudExplorer": "js/CloudExplorer",
        "Common": "js/Common",
        "Providers": "js/Providers",
        "StorageExplorer": "js/StorageExplorer",
        "text": "lib/text/text"
    },
    shim: {
        jquery: {
            exports: "$"
        },
        knockout: {
            deps: ["jquery"],
            exports: "ko"
        },
        "knockout.mapping": {
            deps: ["jquery", "knockout"],
            exports: "mapping"
        }
    },
    waitSeconds: 30 // line added
};

@JasonYeMSFT That worked. Thanks so much!

Nice. We will evaluate the timeout parameter in our next milestone and extend it as needed.

We have shipped a patched version 1.22.1 which bumps all related timeout parameters to 30 seconds.


Add full example of creating a Responsive Search Ad (with keywords and geo targeting)

Also includes minor cleanup of reformatting campaignmanagement/CreateExperiment using google-java-format.

Will revisit this once we've updated the approach for image extensions


[Feature Request] - Automated email Recommendation

Would it be possible for us to scan emails and store all email addresses it encounters into a database. Then recommend emails as you type into To, Cc or BCc fields.

I think you'd want K-9 to become a proper contact provider. I'm not completely opposed to it, but it's quite a lot of work.

That would be just great. But this suggestion system is a much required functionality and won't be half bad if we can do it using a incident tally table and then slowly build the contact system around.

Duplicate of #1006


[FEATURE REQUEST] Style/SingleSpaceBeforeFirstArg:AllowForAlignment

There exists this configuration option for Style/ExtraSpacing:
Style/ExtraSpacing:
  AllowForAlignment: true

Can we add the same configurability for Style/SingleSpaceBeforeFirstArg ?
Useful example:
.user-form
  = simple_form_for User.new do |f|
    = f.inline_input   :full_name,     as: :string
    = f.disabled_input :password,      as: :password
    = f.masked_input   :zip_code,      as: :string
    = f.masked_input   :email_address, as: :email
    = f.masked_input   :phone_number,  as: :tel

Just pushed a fix for this one.

Awesome! Thanks!

@alexdowad I've been getting this warning, but I'm using the most recent release (0.36)
Warning: unrecognized cop Style/SingleSpaceBeforeFirstArg found in foo/.rubocop.yml
Thoughts? Thanks

It's called SpaceBeforeFirstArg now. If my open PR is merged, RC will print a more helpful message. (See 6397b14.)

Ahhh! Gotcha, thanks!


Add a type for logical literals

Wait, what? What did we support before?

Wait, what? What did we support before?

We have always supported the type logical. This change support for logical literals, such as true and false, into the type system as child types of logical. Previously if you did a hover over let foo = true in foo it would say the type is boolean. Now it'll say true.


_elements option should not be applied to the _include option on a search.

Describe the bug
A clear and concise description of what the bug is.
When issuing a search and specifying the _elements option to include specific elements only, it should not be applied to the resources returned due to the _include option.  https://www.hl7.org/fhir/search.html#elements
Currently it is applied to all returned resources.
To Reproduce
Steps to reproduce the behavior:

Go to '...'
Click on '....'
Scroll down to '....'
See error
Run a query with _elements=identifier&_include=:*

Expected behavior
A clear and concise description of what you expected to happen.
Additional context
Add any other context about the problem here.

#1494 should be tackled at the same time.

stretch goal for sprint 21

Verified that with this fix, the _elements is not applied to the included resources.


Format ci

Run dotnet-format, with the --check argument which fails if it finds any files violating the style rules. See https://github.com/feliwir/veldrid/pull/1 for an example (only Buffer.cs is bad formatted). I was looking into more sophisticated actions e.g. https://github.com/marketplace/actions/dotnet-format , but these have some issues currently.
Using those would allow only checking the diff and commenting on the PR. Question is if we should commit the changes immediatly.

Requires #393 to pass of course


Properly set quic tls variable to openssl on windows

The string wasn't cached, so it was being overwritten

You're right that it was supposed to be using openssl per the initial cmake, but curl folks actually want schannel on Windows. So we need:

Make this configurable so an msh3 consumer can pick between schannel and openssl on Windows.
Default to schannel on Windows.
Add Windows build options for both schannel and openssl variants in CI.


#13 add callable support

Fixes #13
@crhdev I think this approach is a bit more backwards compatible.
Your proposed change in https://github.com/crhdev/cakephp-scheduler/commit/83b584123a7cd5412eb47060671180028aeb2278 changed a bit too much for my likeing.
My approach creates an anomyous command which can't be executed via bin/cake but still fits into how the rest of the scheduler system works.

Yes, this approach is ok for me.

After CakePHP 5.1 is released I'm going to do a 1.1 release for this plugin

@crhdev see https://github.com/LordSimal/cakephp-scheduler/releases/tag/1.1.0


Update Stax Icons

Use more kaspa-like stax icons as opposed to perfectly round ones to match branding

Codecov Report
Patch and project coverage have no change.

Comparison is base (58aa502) 93.39% compared to head (2dffd98) 93.39%.

:exclamation: Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.
Additional details and impacted files
@@           Coverage Diff           @@
##             main      #47   +/-   ##
=======================================
  Coverage   93.39%   93.39%
=======================================
  Files          23       23
  Lines        1287     1287
=======================================
  Hits         1202     1202
  Misses         85       85

Flag
Coverage Δ

unittests
93.39% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.


Issue with blog page missing image (?)

This is a Bug Report
Problem:
missing screenshot or issue on my side?

/area blog
/priority backlog

It's likely that the article referenced an external image that is now gone.


Will it be ever updated? :)

More icons, good starting and extraodrinary pack

Yeah, absolutely good design, just need more effort :(


LaTeX in Basic Income Builder isn't showing up

Either locally or on the website, though it was working for me previously

Fixes again in #257


Online doxygen reference still at 1.4.0, not 1.5.0

The online doxygen API docs at https://raspberrypi.github.io/pico-sdk-doxygen/ have not been updated to 1.5.0.

These are deprecated as we're in the process of integrating them directly into the mainline documentation. See raspberrypi/documentation#2743 for details. I'd expect this to be live next week, potentially the week after, as a lot of us are on holiday for the February half-term school break right now. After that the existing location will redirect folks to the new pages.

Thanks for the info.  Have a good break, you guys have earned it!

I'd hoped to have it live before 1.5.0 was released, alas not! 🤷♂

Should go live on Monday, we just figured out the last of the bugs for the new site.


Develop

closes #195
closes #87
closes #111
closes #167
closes #202

Codecov Report

Merging #203 (abfdd04) into main (a2681e0) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##              main      #203    +/-   ##
==========================================
  Coverage   100.00%   100.00%
==========================================
  Files           34        34
  Lines         4774      4975   +201
  Branches      1078      1151    +73
==========================================
+ Hits          4774      4975   +201

Flag
Coverage Δ

unittests
100.00% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files
Coverage Δ

src/classes/simple-calendar-notes.ts
100.00% <ø> (ø)

src/constants.ts
100.00% <ø> (ø)

src/classes/api.ts
100.00% <100.00%> (ø)

src/classes/date-selector.ts
100.00% <100.00%> (ø)

src/classes/game-settings.ts
100.00% <100.00%> (ø)

src/classes/predefined-calendar.ts
100.00% <100.00%> (ø)

src/classes/season.ts
100.00% <100.00%> (ø)

src/classes/simple-calendar-configuration.ts
100.00% <100.00%> (ø)

src/classes/simple-calendar.ts
100.00% <100.00%> (ø)

src/classes/time.ts
100.00% <100.00%> (ø)

... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a2681e0...abfdd04. Read the comment docs.


Remove the 32mbytes max proposal payload size restriction

Colfer has this max serialized size restriction on colfer defined types, we inherited that. No user has raised any concern yet but this is obviously not something desirable.
Remove the 32mbytes max proposal payload size restriction.

The above described restriction has been removed in cd5dfedbede9b3d574973773cf7a9db6f5a5b653
ErrPayloadTooBig is now only returned when the proposal payload is larger than user specified Config.MaxInMemLogSize.


Model trained on GPU cannot be used to predict on CPU

Description
I encountered an error when trying to use a trained model for prediction. The model was initially trained using a GPU, but when attempting to run predictions using the same model on a CPU, an error occurs. The error message indicates that Torch was not compiled with CUDA enabled, despite the prediction being run on a CPU.
Additionally, I used Machamp for a multilabel classification task.
Steps to Reproduce

Train a model by using GPU
Predict using earlier model, in CPU

Expected Behavior
The model should be able to predict as normal.
Actual Behavior
Command I used to predict:
python3 predict.py models/model_name.pt data/test.tsv predictions/prediction.out --device -1

Machamp returns the following error:
Traceback (most recent call last):
  File "/Users/redacted/Code/machamp/predict.py", line 55, in <module>
    predict_with_paths(model, input_path, output_path, args.dataset, args.batch_size, args.raw_text, device, args.conn, args.sep, args.threshold)
  File "/Users/redacted/Code/machamp/machamp/predictor/predict.py", line 228, in predict_with_paths
    write_pred(out_file, batch, device, dev_dataset, model, data_config[dataset], raw_text, conn, sep)
  File "/Users/redacted/Code/machamp/machamp/predictor/predict.py", line 194, in write_pred
    out_dict = model.get_output_labels(enc_batch['token_ids'], enc_batch['golds'], enc_batch['seg_ids'],
  File "/Users/redacted/Code/machamp/machamp/model/machamp.py", line 488, in get_output_labels
    out_dict[task] = self.decoders[task].get_output_labels(mlm_out_task, task_word_mask, golds_task)
  File "/Users/redacted/Code/machamp/machamp/model/multiclas_decoder.py", line 49, in get_output_labels
    logits = self.forward(mlm_out, mask, gold)['logits']
  File "/Users/redacted/Code/machamp/machamp/model/multiclas_decoder.py", line 39, in forward
    self.metric.score(preds[:,1:], gold.eq(torch.tensor(1.0, device=self.device))[:, 1:], None)
  File "/usr/local/Caskroom/miniconda/base/envs/machamp/lib/python3.10/site-packages/torch/cuda/__init__.py", line 293, in _lazy_init
    raise AssertionError("Torch not compiled with CUDA enabled")
AssertionError: Torch not compiled with CUDA enabled

Environment
Training Environment
Google Colab Pro, Python 3.10.12
Prediction Environment
macOS Monterey 12.6, Python 3.10.12
Possible Cause
MachampModel class has an MLM and multiple decoders. During prediction, while the MLM itself has been moved to CPU, the encoders have not. Thus, while trying to predict, the decoders assume that they're still running on GPU, causing the mentioned error.
Currently, I fixed this issue locally by modifying get_output_labels function on MachampModel:
...
# move decoder to CPU before prediction
self.decoders[task].device = self.device

out_dict[task] = self.decoders[task].get_output_labels(mlm_out_task, task_word_mask, golds_task)
...

Thanks for the detailed report and fix!, the fix seems accurate and at the right location. Could you submit a pull request?, otherwise I will merge it into the next update


Plugin in android

Is it possible to make this plugin work in android?
By the way, it is very useful plugin!

I'm afraid not. This plugin is based on glob of node, neither of which works on mobile.


feat(PageHero): don't hide video on xs screens

Overview
as a parting gift to us, Dev redesigned the video hero to address accessibility concerns. The video is no longer hidden on xs screens and instead just wraps to the next line
I also refactored slightly so that column sizes for the left and right hero columns are defined in the same file

PR Checklist

[x] Related to designs
[x] Related to JIRA ticket: REACH-623
[x] I have run this code to verify it works
[ ] This PR includes unit tests for the code change

📬Published Alpha Packages:@codecademy/gamut-kit@0.2.17-alpha.979063.0@codecademy/gamut-labs@11.4.0-alpha.979063.0<EMAIL_ADDRESS>
🚀 Styleguide deploy preview ready!https://604fdb7a89994b171534e18a--gamut-preview.netlify.appDeploy Logs


integration-cli: add cli/api stats tests when container not found

kind related to #16361
and making sure we don't regress on this
Signed-off-by: Antonio Murdaca<EMAIL_ADDRESS>

LGTM if janky is happy

LGTM, just waiting :)


Remove repo?

Hi!
It would be great to remove this repo so the one under the Jenkins organization (https://github.com/jenkinsci) becomes the source of all forks.
This repo isn't getting any activity, so I figured that it would be a good idea to delete it. That way users won't see any "forked from" in the repo linked in the Jenkins UI.
Thanks!

Hi Miguel!
I have no problem with that, especially if it's causing confusion for the real repo.

Dick

Thanks Dick! I think it will be a good idea. The Jenkins plugin guidelines recommend it, too.
Cheers,
Miguel

Ok, I'll delete it in the next few days


Theme switcher does not appear to function (wasm)

Describe the bug

When selecting various themes in the Display section, no theme switch occurs. Screenshot of console attached. (Uncaught Error: Method not implemented)
Environment
wasm, chrome (Version 120.0.6099.216 (Official Build) (64-bit))
Code to reproduce
Navigate to marimo.new and then try switching the theme.

Ah yea, I haven't implemented the user settings yet. Should be an easy fix though (May not get to it until next week)


Port with type any cannot connect to port with an array type (alias)

Related to this, when manually added the connection to .syn file, then also the types are not solved properly and something similar (type casting) is generated:
test_iv_0.t.value = (any)alias_test_iv_0_outputs_47[0];


Release proxybroker2 module to pypi

Summary
Hi @bluet, inspired from https://github.com/bluet/proxybroker2/issues/28, but we need to complete the following pre-work

[ ] Revise the pack scripts setup.py
[ ] Revise the dependency
[ ] Clear codebase architecture and documentation
[ ] Clear define how to contribute to this repo
[ ] Roadmap
[ ] Add coverage-test e.g. pytest-cov
[ ] Add workflow to test package on different platforms e.g. Windows and macOS
[ ] De-fork

regarding to setup.py scripts, maybe we should use poetry?

@a5r0n migrating to poetry would be nice 👍

poetry is one of my potential solution, but we require to clarify the dependencies on all Python version we supported before doing so

i just opened a PR for the poetry migration part, can u guys have a look?

Ref #86


Pattern with globstar and star fails to match

assert!(glob_match("**/*.js", "a/b.c/c.js"));

assertion failed: glob_match(\"**/*.js\", \"a/b.c/c.js\")
thread 'tests::abc' panicked at 'assertion failed: glob_match(\"**/*.js\", \"a/b.c/c.js\")'

glob_match returns false even though it should return true. This has something to do with the . in b.c part. If I remove the dot, the glob matches.

The issue comes from the folder that matches the wildcard glob. The following assertion is failing:
assert!(glob_match("**/*a", "a/a"));

Related to https://github.com/web-infra-dev/rspack/issues/6245 also.


Tidy up and package for NuGet as v1.0.0

Package this project up ready for release as version 1.0.0

[ ] Use git filter-branch to remove all unwanted dll files
[ ] Remove all other projects
[ ] Rewrite README
[ ] Create Nuspec file
[ ] Remove unwanted files in Common and util
[ ] Rewrite AssemblyInfo.cs
[ ] Bump version number in solution and project files
[ ] Ensure documentation is built
[ ] Replace .gitignore
[ ] Rename Solution file and test project

Blocked by the release of CSF.Data.


Debian LVM support broken by PR 255

I've recently upgraded my FreeNAS system from 9.10 to 11.1. In doing so, I've received an update from iohyve 0.7.7 to 0.7.9. My existing Debian LVM VMs fail to start in 11.1, falling through to the grub prompt. After sifting through source code, I can see that when Ubuntu support was added (#255), d8lvm was borked.
Looking at the changes, I can see that an additional switch was correctly created for ubuntu for the case where $install = "yes" (line 369 in latest source code), but replaced d8lvm for the case where $install = "no" (line 425).
Consequently, when trying to run a VM with os="d8lvm", execution falls out of the os check and into a default grub-bhyve statement, which is incorrect for starting a Debian LVM VM. End result is a grub prompt rather than a running VM.
I think I will be able to get my VMs going under 11.1 by using os="ubuntu", but I haven't tested this yet, and am concerned there may be side effects. It would be good to get the d8lvm support corrected.

I've just gotten Ubuntu working, I tracked down the issue to Ubuntu installs on (hd0,gpt2) but bhyve is started with (hd0,msdos1).  I modified the ioh-guest code to force it to be (hd0,gpt2) and it booted.


Bumped development release number for 1.26

[Issue #2265] (https://github.com/aws/eks-distro/issues/2265)
New development release for 1.26
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

/lgtm

/unhold
/approve


plot on output widget?

Hi - great library! Would it be possible to plot on an ipywidget output widget?
I tried, and only the plot appears, but it won't update.

Hi, thanks! Unfortunately, I know very little about ipywidget and can not really help here. You might want to checkout out the lrcurve library which serves as a basis for this library. Maybe Andreas Madsen can help you out. Let me know if you find a solution!

thanks for the response - will check there.

Thanks for the reference and good luck with your project. Closing this for now.


PSR-1 standards check

Hi Leon,
Here you can find PSR-1 standards https://www.php-fig.org/psr/psr-1/
I found some issues with following to PRS-1 standard in your code. Here are some of them:

Files MUST use only "
Namespaces and classes MUST follow an "autoloading" PSR: [PSR-0, PSR-4]. - In my commit into branch psr-1 you yon find how to use namespaces
Please also check this "Class names MUST be declared in StudlyCaps." also folers has to start with uppercase
Method names MUST be declared in camelCase().

I think i have updated all the existing files to the psr-1 standard


Adding master election interface to topo.Impl.

Along with unit test.
Implementing it for zookeeper and etcd.
@michael-berlin @pivanof will work on internal implementation as you guys review this, so if you don't like the interface, please get back to me asap... thanks!

This change is

Reviewed 9 of 10 files at r1, 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 14 unresolved discussions.

go/vt/topo/server.go, line 293 [r2] (raw file):

	//
	// Master election, local to multiple cells usually.

What does "local to multiple cells" mean?

go/vt/topo/server.go, line 298 [r2] (raw file):

	//

	// NewParticipation starts participating in the named group,

s/NewParticipation/NewMasterParticipation/

go/vt/topo/server.go, line 298 [r2] (raw file):

	//

	// NewParticipation starts participating in the named group,

Both implementations of MasterParticipation don't actually start participating in the election until WaitForMastership() is called, and according to the test that behavior seem to be deliberate. Can you please fix the comment to reflect that?

go/vt/topo/server.go, line 346 [r2] (raw file):

	// the returned context will be cancelled. If Shutdown was called,
	// WaitForMaster will return nil, ErrInterrupted.
	WaitForMaster() (context.Context, error)

WaitForMastership?

go/vt/topo/test/election.go, line 15 [r2] (raw file):

)

func waitFormasterID(t *testing.T, mp topo.MasterParticipation, expected string) {

Please capitalize Master in waitFormasterID.

go/vt/topo/test/election.go, line 27 [r2] (raw file):

		}

		c--

This seems to be a very primitive and imprecise time measurement. Can we use normal deadlines?

go/vt/topo/test/election.go, line 32 [r2] (raw file):

		}

		t.Logf("Unexpected master %v, expected %v, will retry", master, expected)

Printing this line 100 times a second is not nice. Can we print it once at the beginning of waiting?

go/vt/topo/test/election.go, line 37 [r2] (raw file):

}

// checkElection runs the tests on the MasterParticipation part of the API

The MasterParticipation part of which API? Can you expand the comment?

go/vt/topo/test/election.go, line 51 [r2] (raw file):

	waitFormasterID(t, mp1, "")

	// wait for it to be the master

"it" is mp1 here?

go/vt/topo/test/election.go, line 83 [r2] (raw file):

	// this should have closed ctx1
	timer := time.NewTimer(5 * time.Second)

Why it is guaranteed that mp1 will close context in 5 seconds?

go/vt/topo/test/faketopo/faketopo.go, line 20 [r2] (raw file):

type FakeTopo struct{}

// Close is part of the topo.Server interface.

Can these changes be separated at least into a separate commit? They don't have anything to do with MasterParticipation change and there are a lot of these.

go/vt/zktopo/election.go, line 1 [r2] (raw file):

// Copyright 2013, Google Inc. All rights reserved.

2016?

go/vt/zktopo/election.go, line 114 [r2] (raw file):

//   being the master.
// - wait for mp.shutdown.
func (mp *zkMasterParticipation) whileMaster(proposal string, cancel context.CancelFunc) {

"whileMaster" seem to be a bad function name not indicating the purpose of the function. Not sure what's the best name would be, but maybe WaitForShutdown() or HoldMastership()?

go/zk/zkconn.go, line 18 [r2] (raw file):

	log "github.com/golang/glog"
	zookeeper "github.com/samuel/go-zookeeper/zk"
	"golang.org/x/net/context"

Why this was moved?

Comments from Reviewable

Review status: 6 of 10 files reviewed at latest revision, 14 unresolved discussions.

go/vt/topo/server.go, line 293 [r2] (raw file):
Previously, pivanof (Pavel Ivanov) wrote…
> What does "local to multiple cells" mean?

Done.

go/vt/topo/server.go, line 298 [r2] (raw file):
Previously, pivanof (Pavel Ivanov) wrote…
> s/NewParticipation/NewMasterParticipation/

Done.

go/vt/topo/server.go, line 298 [r2] (raw file):
Previously, pivanof (Pavel Ivanov) wrote…
> Both implementations of MasterParticipation don't actually start participating in the election until WaitForMastership() is called, and according to the test that behavior seem to be deliberate. Can you please fix the comment to reflect that?

Done.

go/vt/topo/server.go, line 346 [r2] (raw file):
Previously, pivanof (Pavel Ivanov) wrote…
> WaitForMastership?

Done.

go/vt/topo/test/election.go, line 15 [r2] (raw file):
Previously, pivanof (Pavel Ivanov) wrote…
> Please capitalize Master in waitFormasterID.

Done.

go/vt/topo/test/election.go, line 27 [r2] (raw file):
Previously, pivanof (Pavel Ivanov) wrote…
> This seems to be a very primitive and imprecise time measurement. Can we use normal deadlines?

Done.

go/vt/topo/test/election.go, line 32 [r2] (raw file):
Previously, pivanof (Pavel Ivanov) wrote…
> Printing this line 100 times a second is not nice. Can we print it once at the beginning of waiting?

I just needed printed it for debugging when I was coding it, removing it.

go/vt/topo/test/election.go, line 37 [r2] (raw file):
Previously, pivanof (Pavel Ivanov) wrote…
> The MasterParticipation part of which API? Can you expand the comment?

This whole package is testing topo.Impl API, so that's kinda implicit. Extended the comment anyway.

go/vt/topo/test/election.go, line 51 [r2] (raw file):
Previously, pivanof (Pavel Ivanov) wrote…
> "it" is mp1 here?

Done.

go/vt/topo/test/election.go, line 83 [r2] (raw file):
Previously, pivanof (Pavel Ivanov) wrote…
> Why it is guaranteed that mp1 will close context in 5 seconds?

Done.

go/vt/topo/test/faketopo/faketopo.go, line 20 [r2] (raw file):
Previously, pivanof (Pavel Ivanov) wrote…
> Can these changes be separated at least into a separate commit? They don't have anything to do with MasterParticipation change and there are a lot of these.

A bit annoying to separate them now. I discovered that when I was adding the new API, so the file was already opened in my change...

go/vt/zktopo/election.go, line 1 [r2] (raw file):
Previously, pivanof (Pavel Ivanov) wrote…
> 2016?

Done.

go/vt/zktopo/election.go, line 114 [r2] (raw file):
Previously, pivanof (Pavel Ivanov) wrote…
> "whileMaster" seem to be a bad function name not indicating the purpose of the function. Not sure what's the best name would be, but maybe WaitForShutdown() or HoldMastership()?

Done.

go/zk/zkconn.go, line 18 [r2] (raw file):
Previously, pivanof (Pavel Ivanov) wrote…
> Why this was moved?

By convention,  we should have all standard go imports in a section, then all non-vitess ones, then all internal ones. Just fixing that.

Comments from Reviewable

Reviewed 4 of 4 files at r3.
Review status: all files reviewed at latest revision, 16 unresolved discussions.

go/vt/topo/server.go, line 298 [r2] (raw file):
Previously, alainjobart (Alain Jobart) wrote…
> Done.

Can you still add a comment about when the participation in the election is supposed to start? Either here or at WaitForMastership() comment.

go/vt/topo/server.go, line 295 [r3] (raw file):

	// Master election methods. This is meant to have a small number of
	// processes elect a master within a group. The backend storage for this
	// can either be the global topo server, or a chorum of local cells.

I can't find translation of the word "chorum". What does it mean?

go/vt/topo/server.go, line 349 [r3] (raw file):

	WaitForMastership() (context.Context, error)

	// Shutdown is called when we don't want to participate in the

I've just realized that this API assumes that participation in the master election is always started at the start of the application and ends at shutdown. But it's not always true, right? Even the comment here says "called when we don't want to participate", that doesn't necessarily means it's a shutdown of the application. You can argue that it's a shutdown of participation, but I think that's not really an appropriate word for that. Besides that actually it's not clear now when participation in the master election starts and when it ends. It looks like your intention was that participation starts on the first call to WaitForMastership(), but then when the returned context is canceled (i.e. we lost mastership) does participation continues? Or should it stop? Can you clarify with comments where the intended boundaries are and clarify the API itself for that, e.g. Shutdown() should probably be Stop().

go/vt/topo/test/election.go, line 83 [r2] (raw file):
Previously, alainjobart (Alain Jobart) wrote…
> Done.

So it's part of API guarantee that it must always close the context within 5 seconds of the call to Shutdown()? If yes can you add this requirement to the API comments, so that anyone implementing it knew what they have to provide?

go/vt/topo/test/faketopo/faketopo.go, line 20 [r2] (raw file):
Previously, alainjobart (Alain Jobart) wrote…
> A bit annoying to separate them now. I discovered that when I was adding the new API, so the file was already opened in my change...

Why annoying? Just create a commit with these changes and rebase this branch on top of that commit.

go/zk/zkconn.go, line 18 [r2] (raw file):
Previously, alainjobart (Alain Jobart) wrote…
> By convention,  we should have all standard go imports in a section, then all non-vitess ones, then all internal ones. Just fixing that.

Can you extract this into a different PR or at least different commit? It's completely unrelated.

Comments from Reviewable

Review status: 6 of 10 files reviewed at latest revision, 16 unresolved discussions.

go/vt/topo/server.go, line 298 [r2] (raw file):
Previously, pivanof (Pavel Ivanov) wrote…
> Can you still add a comment about when the participation in the election is supposed to start? Either here or at WaitForMastership() comment.

Done.

go/vt/topo/server.go, line 295 [r3] (raw file):
Previously, pivanof (Pavel Ivanov) wrote…
> I can't find translation of the word "chorum". What does it mean?

quorum. dunno why I spelled it so wrong...

go/vt/topo/server.go, line 349 [r3] (raw file):
Previously, pivanof (Pavel Ivanov) wrote…
> I've just realized that this API assumes that participation in the master election is always started at the start of the application and ends at shutdown. But it's not always true, right? Even the comment here says "called when we don't want to participate", that doesn't necessarily means it's a shutdown of the application. You can argue that it's a shutdown of participation, but I think that's not really an appropriate word for that. Besides that actually it's not clear now when participation in the master election starts and when it ends. It looks like your intention was that participation starts on the first call to WaitForMastership(), but then when the returned context is canceled (i.e. we lost mastership) does participation continues? Or should it stop? Can you clarify with comments where the intended boundaries are and clarify the API itself for that, e.g. Shutdown() should probably be Stop().

Added more comments and renamed to Stop, you're right it's clearer.

go/vt/topo/test/election.go, line 83 [r2] (raw file):
Previously, pivanof (Pavel Ivanov) wrote…
> So it's part of API guarantee that it must always close the context within 5 seconds of the call to Shutdown()? If yes can you add this requirement to the API comments, so that anyone implementing it knew what they have to provide?

the comment should say 'as soon as possible', there is no time expectation. 5s is just the test timing out when it becomes unreasonable.

go/zk/zkconn.go, line 18 [r2] (raw file):
Previously, pivanof (Pavel Ivanov) wrote…
> Can you extract this into a different PR or at least different commit? It's completely unrelated.

so where do we stop and create different commits here? a typo in a comment ten lines earlier should also be a different commit? this is a bit extreme, I'd rather fix very minor stuff like this as I find it and just take it as part of the commit... then event though extracting it to a different commit is possible, it feels like a huge waste of time... so when it's convenient, I'll do it, but this is a bit late for this one.

Comments from Reviewable

Sorry that it took me so long to review it.
I'm fine with the API and the different implementations after my comments are addressed.
Overall, I think this is the right way to do it and we can refine it if necessary along the way.

Reviewed 6 of 10 files at r1, 4 of 4 files at r4.
Review status: all files reviewed at latest revision, 28 unresolved discussions.

go/vt/etcdtopo/election.go, line 32 [r4] (raw file):

// etcdMasterParticipation implements topo.MasterParticipation.
//
// We create a single file and use etcd's compare&swap with TTLs.

Please mention here that the lock file will be per name and that the content of the file will be the id of the current master.

go/vt/etcdtopo/election.go, line 107 [r4] (raw file):

		// Wait for the lock file to be deleted, then try again.
		ctx, cancel := context.WithCancel(context.Background())
		go func() {

Add a comment here that the sole purpose of this go routine is to cancel out the wait if Stop() is called.

go/vt/topo/server.go, line 346 [r2] (raw file):
Previously, alainjobart (Alain Jobart) wrote…
> Done.

How about `StartAndWaitForMastership`? This would be in line with the new `Stop` method.
Alternatively, something shorter e.g. StartAndWait.

go/vt/topo/server.go, line 295 [r3] (raw file):
Previously, alainjobart (Alain Jobart) wrote…
> quorum. dunno why I spelled it so wrong...

Can you please explain what you mean with `local cells`? I interpret `cell` with `data center` and I don't think that makes sense here?

go/vt/topo/server.go, line 302 [r3] (raw file):

	// Id is the name of the local process, passing in the hostname:port of the
	// current process as id is the common usage.
	NewMasterParticipation(name, id string) (MasterParticipation, error)

Please clarify in the comments that id must be unique between different (running) master processes for the same name.

go/vt/topo/server.go, line 345 [r3] (raw file):

	// WaitForMastership waits until this process is the master.
	// After we become the master, we may lose mastership. In that case,
	// the returned context will be cancelled. If Shutdown was called,

Please write canceled with a single l. The Go team recently switched to this spelling. Therefore, let's try to use that spelling as well.

go/vt/topo/server.go, line 349 [r3] (raw file):
Previously, alainjobart (Alain Jobart) wrote…
> Added more comments and renamed to Stop, you're right it's clearer.

I was about to suggest `Release` or `Relinquish` :)
But I think we should use Stop because the internal API uses that as well.

go/vt/topo/test/election.go, line 87 [r4] (raw file):

	select {
	case <-ctx1.Done():
	case <-timer.C:

You can use time.After(5 * time.Second) here instead and get rid of the timer object.

go/vt/topo/test/testing.go, line 96 [r4] (raw file):

	ts.Close()

	t.Log("=== checkElection")

I'm not a big fan of such banner comments but I understand the need for it here.
Maybe add another one on top of checkVSchema as well to be consistent?

go/vt/zktopo/election.go, line 51 [r4] (raw file):

// zkMasterParticipation implements topo.MasterParticipation.
//
// We use a directory with files created as sequence and ephemeral,

Please also mention here that there will be one directory per name.

go/vt/zktopo/election.go, line 77 [r4] (raw file):

	select {
	case <-mp.stop:
		close(mp.done)

Multiple calls to WaitForMastership after Stop will panic here because the channel is already closed.
If we say that you should not call WaitForMastership after Stop, that's fine with me. In that case, you may need to clarify the API description in server.go.
There are multiple call sites close(mp.done) in this function and watchMastership which make it hard to reason whether we actually caught all cases.
What about the following alternative: Use a watch group which you increment in this function and also before calling go mp.watchMastership? You can add a defer to both functions as well. The count changes would look like this:
WaitForMastership() -> 1
go mp.watchMastership() -> 2
defer from WaitForMasterShip() -> 1
defer from watchMastership() -> 0

(The same comment applies to the etcd implementation as well.)

go/vt/zktopo/election.go, line 90 [r4] (raw file):

24*time.Hour

Let's use a shorter timeout here to make sure that the timeout retry code gets exercised during manual testing or as part of canarying? e.g. something like 5 minutes?

go/vt/zktopo/election.go, line 127 [r4] (raw file):

	_, stats, events, err := mp.zkts.zconn.GetW(proposal)
	if err != nil {
		log.Warningf("Cannot watch proposal whle being master, stopping: %v", err)

typo: while

go/vt/zktopo/election.go, line 139 [r4] (raw file):

			log.Warningf("Error deleting our proposal %v: %v", proposal, err)
		}
		close(mp.done)

How about calling this by a defer? e.g. I'm wondering if the case below should close mp.done as well.

go/vt/zktopo/election.go, line 153 [r4] (raw file):

}

// GetCurrentMasterID is part of the topo.MasterParticipation interface

I would prefer a short comment here where you clarify that the node with the smallest sequence number in its name is the current master.

go/zk/zkconn.go, line 18 [r2] (raw file):
Previously, alainjobart (Alain Jobart) wrote…
> so where do we stop and create different commits here? a typo in a comment ten lines earlier should also be a different commit? this is a bit extreme, I'd rather fix very minor stuff like this as I find it and just take it as part of the commit... then event though extracting it to a different commit is possible, it feels like a huge waste of time... so when it's convenient, I'll do it, but this is a bit late for this one.

I agree with Alain on this one. We should not enforce this for our pull requests. It should be up to the author, not the reviewer to factor out small changes like this.
In the past I did this very often (Eclipse has a nice Git Staging view which makes it very easy to move changed parts of a file to the Git index). But after all, I found that it's not worth the time. Therefore, I'll probably do this less and less.

Comments from Reviewable

Review status: all files reviewed at latest revision, 28 unresolved discussions.

go/vt/etcdtopo/election.go, line 32 [r4] (raw file):
Previously, michael-berlin (Michael Berlin) wrote…
> Please mention here that the lock file will be per `name` and that the content of the file will be the `id` of the current master.

Done.

go/vt/etcdtopo/election.go, line 107 [r4] (raw file):
Previously, michael-berlin (Michael Berlin) wrote…
> Add a comment here that the sole purpose of this go routine is to cancel out the wait if Stop() is called.

Done.

go/vt/topo/server.go, line 346 [r2] (raw file):
Previously, michael-berlin (Michael Berlin) wrote…
> How about `StartAndWaitForMastership`? This would be in line with the new `Stop` method.
>
> Alternatively, something shorter e.g. `StartAndWait`.

It's not symmetrical to Stop, actually. WaitForMastership is called in a loop, Stop is called at the end. So I don't think we should have Start in the name... NewMasterParticipation is the Stop() equivalent.

go/vt/topo/server.go, line 295 [r3] (raw file):
Previously, michael-berlin (Michael Berlin) wrote…
> Can you please explain what you mean with `local cells`? I interpret `cell` with `data center` and I don't think that makes sense here?

Done.

go/vt/topo/server.go, line 302 [r3] (raw file):
Previously, michael-berlin (Michael Berlin) wrote…
> Please clarify in the comments that `id` must be unique between different (running) master processes for the same `name`.

Done.

go/vt/topo/server.go, line 345 [r3] (raw file):
Previously, michael-berlin (Michael Berlin) wrote…
> Please write `canceled` with a single `l`. The Go team recently switched to this spelling. Therefore, let's try to use that spelling as well.

I'm trying, I think it's the only place I missed it. ;)

go/vt/topo/test/election.go, line 87 [r4] (raw file):
Previously, michael-berlin (Michael Berlin) wrote…
> You can use `time.After(5 * time.Second)` here instead and get rid of the `timer` object.

Done.

go/vt/topo/test/testing.go, line 96 [r4] (raw file):
Previously, michael-berlin (Michael Berlin) wrote…
> I'm not a big fan of such banner comments but I understand the need for it here.
>
> Maybe add another one on top of `checkVSchema` as well to be consistent?

it is there?

go/vt/zktopo/election.go, line 51 [r4] (raw file):
Previously, michael-berlin (Michael Berlin) wrote…
> Please also mention here that there will be one directory per `name`.

Done.

go/vt/zktopo/election.go, line 77 [r4] (raw file):
Previously, michael-berlin (Michael Berlin) wrote…
> Multiple calls to `WaitForMastership` after `Stop` will panic here because the channel is already closed.
>
> If we say that you should not call `WaitForMastership` after `Stop`, that's fine with me. In that case, you may need to clarify the API description in `server.go`.
>
> There are multiple call sites `close(mp.done)` in this function and `watchMastership` which make it hard to reason whether we actually caught all cases.
>
> What about the following alternative: Use a watch group which you increment in this function and also before calling `go mp.watchMastership`? You can add a defer to both functions as well. The count changes would look like this:
>
> ```
> WaitForMastership() -> 1
> go mp.watchMastership() -> 2
> defer from WaitForMasterShip() -> 1
> defer from watchMastership() -> 0
> ```
>
> (The same comment applies to the etcd implementation as well.)

Using a WaitGroup is a great alternative, I will make that change. In a separate PR though, will be easier for me. Already have had this PR in flight for a long time. :)

go/vt/zktopo/election.go, line 90 [r4] (raw file):
Previously, michael-berlin (Michael Berlin) wrote…
> > 24*time.Hour
>
> Let's use a shorter timeout here to make sure that the timeout retry code gets exercised during manual testing or as part of canarying? e.g. something like 5 minutes?

Done.

go/vt/zktopo/election.go, line 127 [r4] (raw file):
Previously, michael-berlin (Michael Berlin) wrote…
> typo: while

Done.

go/vt/zktopo/election.go, line 139 [r4] (raw file):
Previously, michael-berlin (Michael Berlin) wrote…
> How about calling this by a `defer`? e.g. I'm wondering if the case below should close `mp.done` as well.

it should not, because below it is called when we lose mastership, not when stop() was called.

go/vt/zktopo/election.go, line 153 [r4] (raw file):
Previously, michael-berlin (Michael Berlin) wrote…
> I would prefer a short comment here where you clarify that the node with the smallest sequence number in its name is the current master.

Done.

Comments from Reviewable

Reviewed 4 of 4 files at r5.
Review status: all files reviewed at latest revision, 19 unresolved discussions.

go/vt/etcdtopo/election.go, line 112 [r5] (raw file):

		go func() {
			// This go routine cancels out the context if Stop()
			// it called.

typo: is

go/vt/topo/server.go, line 346 [r2] (raw file):
Previously, alainjobart (Alain Jobart) wrote…
> It's not symmetrical to Stop, actually. WaitForMastership is called in a loop, Stop is called at the end. So I don't think we should have Start in the name... NewMasterParticipation is the Stop() equivalent.

`NewMasterParticipation` only initializes the object but e.g. doesn't interact with the topology yet.
This is delayed until this method. IMO, this method does two things: It starts getting the master lock and it also blocks until the lock is actually obtained. If you would split this method into two methods, they would be called Start and Wait. Hence the proposal StartAndWait.
Also: After NewMasterParticipation is called there is no need to call Stop. But after this method is called (StartAndWait... :)), Stop must be called.

go/vt/topo/server.go, line 302 [r5] (raw file):

	// NewMasterParticipation creates a MasterParticipation
	// object, used to become the Master in an election for the
	// provided group name.  Id is the name of the local process,

nit: double space

go/vt/topo/test/testing.go, line 96 [r4] (raw file):
Previously, alainjobart (Alain Jobart) wrote…
> it is there?

Sorry, the diff view fooled me.

Comments from Reviewable


Helm Auth Config with updated console

Looks like we can totally eliminate the local-values.yaml file with this PR 🎉

Need to change the check in etc/helm/pachyderm/templates/pachd/config-job.yaml to activateEnterprise instead of enterpriseLicenseKey

Can you add docs to both the helm docs and to the values.yaml file for the new fields?


granite: 5.2.2 -> 5.2.3

Motivation for this change
Things done

[x] Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
Built on platform(s)

[x] NixOS
[ ] macOS
[ ] other Linux distributions

[ ] Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
[ ] Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
[ ] Tested execution of all binary files (usually in ./result/bin/)
[ ] Determined the impact on package closure size (by running nix path-info -S before and after)
[ ] Assured whether relevant documentation is up to date
[ ] Fits CONTRIBUTING.md.

I've already bumped this in staging-next, though thanks for them in general.
Though I haven't bumped gala yet :wink:


feat: Sender(..., tls=True) now also uses OS-provided certificate store.

Depends on: https://github.com/questdb/c-questdb-client/pull/47


Convert the server DB tests to use GORM with SQLite3 instead of sqlmock

In preparation for gorm-izing the DB itself.
This way we have passing tests we know work against the previous non-gorm storage package before gorm-izing the storage package.

Love the test changes coming first, looks good at a quick glance! Will look closer over the weekend.

Actually I'm wondering if I approached this wrong - should I have written these tests only in terms of the MetaStore api (like the memory storage), so that the tests can be shared between any storage implementation (such as both sql and memory)?

@cyli that is the optimal way to do it but you sometimes end up with split code as the setup can call the interface functions and be shared, but sometimes you want the validation to dig into the internals of an implementation.
Go generally favours a bit of repetition here and there (one of the necessities of not having generics, and interfaces don't fit everything) so this is fine.

@endophage I think all the comments are addressed (if I understood them correctly, anyway!  :)), and I made a stab at adding the migration SQL.
I tried running the initial.sql + migrate.sql, vs just using the gorm create table functions, and there seems to be no difference in schema (the gorm ID seems to be int(10) and initial.sql creates IDs with int(11), so I just altered them to be int(10)).
However, the charset seems to be different - when created with initial.sql, the charset as displayed by show status table where name = '<tablename>' of the tables is utf8_general_ci.  The charset produced using the gorm is utf8_unicode_ci.
Any suggestions on fixing this?  I'm passing the default charset to the table options when creating the GORM table.

The int(10) vs int(11) stuff looks fine. Good to just make it uniform with Gorm so nobody looks back and wonders if there was some special reason for it to differ.
Not sure why the difference in charset... A SHOW CREATE TABLE <tablename> on Gorm vs manually created tables just shows "charset=utf8" for both... This SO post has a good first answer, just sorting/comparison differences between the 2 so I'm not particularly concerned.

LGTM!

@endophage Oops, sorry!  It collate, not charset.  Should I add that to the gorm table options, or do we not care?

@cyli yeah, I had a feeling it might have been collation. Don't really care in this instance. The only thing we might sort on that it would affect is the gun, and I don't see us every actually sorting on that for anything but internal analytics, at which point we can deal with sorting possibly being a little out of order.


[SC] Add all members of the SC as part of this repository.

Otherwise we cannot tag them all when an action from the SC is needed.

Just now I've updated the list of the tsc group, and added the tsc group to this repo.
With this change all the members of the SC can now be marked / requested for a review.
Closing the issue as it's solved, but feel free to reopen it in case someone thinks it's needed.


feat: Use finish_reason to check if response messages are tool_calls

[x] I have looked for existing issues (including closed) about this

Feature Request
feat: Use finish_reason to check if response messages are tool_calls
Motivation
https://github.com/0xPlaygrounds/rig/blob/8e6f197f27e0bdbda83d356b7c34001d95a2882e/rig-core/src/providers/openai.rs#L381
Currently we check if tool_calls is None, and actually some openai compatible servers like vllm just return empty list of tool_calls and the openai document does not express tool_calls as optional, so it's not save to just check it with None.
Proposal
Check the finish_reason field on tool_calls or add additional check for empty list
Alternatives
Maybe other ways exist to handle this

This is a good catch. A lot of our implementation for the openai spec actually has changed overtime due to a natural upgrade in LLM specs (all backwards compatible).


Feature/rds cluster resource id output

Fix for issue #27
I also needed this for my AWS setup. So I've forked this and made the modifications to get it working in my staging environment.
Test it by following these guidelines by Amazon:
Creating and Using an IAM Policy for IAM Database Access
Sample policy is in the example file.

Thanks, @melechi !
v1.12.0 has been released.


Desync detected

Product and Version [VS/VSCode]: VSCode
OS Version [macOS/Windows/Linux]: Windows 10
Live Share Extension Version: 1.0.2106
Target Platform or Language [e.g. Node.js]: AngularJS
What were you doing when you encountered the desync?
I had switched to a tab for a file and got the notification.  This was a file I had added from the instance of VSCode that joined the session if that makes a difference.  I had added the file a few minutes before I think.  When it happened before I think it was also a file I added from the client if that makes a difference.  I am only making changes on the client btw, I jiust use live sync to work on code from a second laptop, so I hadn't made any changes on the computer that started the session since I started it.
How often have you encountered desyncs?
every few hours

Thanks for the detailed report. That's definitely interesting info that there's no activity from the host when this happens.
Does this ever happen for you when not adding files from the guest?
If by chance you're able to come up with steps to reproduce this issue, that'd also be enormously helpful (the details you've already provided are quite helpful, thanks!).

Duplicate of #3715


how to get the attributes file when using other dataset?

I want to do the experiments with different datasets? How can I get the attributes file?(.npy file)
Thanks

Hi @Feobi1999,
The attributes file strictly depends on the labels you have in your dataset. A simple choice is to use word embeddings. In this repo, I do not have a general way to extract attributes, but for different datasets, but you can check out this code on how to get word embeddings from gensim given a list of class names.

If I am correct, for the DomainNet dataset you have extracted the word embeddings using the code mentioned above and then L2 normalized them. Kindly let me know if I understand correctly.
Thank you

Hi @shivam-chandhok,
Actually, the author of this work kindly provided them to me. I L2 normalize them anyway within the classifier, in this line.


Bump up minimum required Go version

Summary

Bump minimum required Go version to 1.14

@agnivade in the developer setup guide we have various copy-paste commands that include a go version - these are out of date - please can you update or otherwise fix them?


check onchain balance on swap-out request

adds a balance check on an incoming swap-out request

To properly test this, is it required to patch the swap provider side alone, or also the requester?

To properly test this, is it required to patch the swap provider side alone, or also the requester?

Only the provider side needs to be updated


Implement vm-realize

Implement the vm-realize command. This reads the definition of a virtual machine and creates any missing resources such as ZFS volumes. This will require updates to the configuration schema in order to specify the sizes of ZFS volumes so that the command can create them properly.

As of 2392c1c4d1b164b344c78a4e56fde0adb099ddf0, this is actually documented and not yet implemented :eyes:


main menu, different resolutions - pink and black texture only

if you change the video/gui scale, the main menu becomes a giant pink/black texture.

Noted, thank you.


[Lead space search]: Missing spacing below body

Description
Tested on Safari mobile using React wrapper. When the Lead space search has body copy below the heading, it looks like there's some missing padding between the copy and the bottom of the div. On mobile, there should be 32px padding between the body and the top of the next div.
React wrapper build

Design specs

Component(s) impacted
Lead space search
Browser
Safari
Carbon for IBM.com version
v1.17.0-rc.1
Severity
Severity 3 = The problem is visible or noticeable to users but does not impede the usability or functionality. Affects minor functionality, has a workaround.
Application/website
C4IBM.com
Package
@carbon/ibmdotcom-web-components
CodeSandbox example
https://codesandbox.io/s/github/carbon-design-system/carbon-for-ibm-dotcom/tree/main/packages/web-components/examples/codesandbox/components-react/leadspace-with-search
Steps to reproduce the issue (if applicable)
No response
Release date (if applicable)
No response
Code of Conduct

[X] I agree to follow this project's Code of Conduct
[X] I checked the current issues for duplicate issues

Fixed in https://github.com/carbon-design-system/carbon-for-ibm-dotcom/pull/8673.