text:
Allow to search for a specific nutrient in the nutrition edit mode

What
Allow to search for a specific nutrient in the nutrition edit mode
Why

Save time when inputting complex products

Part of

#503

Mockup

I'm working on this one, I hope it's not taken already

source: gharchive/issue

repo: openfoodfacts/smooth-app

license_type: permissive

license_source: github-api


text:
simple examples for kriging and sequential simulation

Hi, I just found your package and it looks very interesting. I was trying to wrap gslib for my research and I came across your work. Do you have any examples of calling gslib functions like kriging and sequential simulation?

amirdel,
You will find a tutorial for Kriging at https://opengeostat.github.io/pygslib/Tutorial.html. You will find more detail at https://opengeostat.github.io/pygslib/API.html.
There is not simulation implemented yet.

authors: amirdel opengeostat

repo: opengeostat/pygslib

license: mit

license_source: bigquery


text:
Supporting legacy formats (compatability with acrg)

Adding legacy compatability to ObsData object (created when get_obs.. functions are called) to current ACRG formats. This is to mimic a dictionary to allow the dataset to be returned when the site key is used (e.g. ObsData["MHD"] would return the dataset - equivalent to ObsData.data). This is to ensure initial legacy compatibility with current ACRG formats which can be depreceated over time and then replaced.
This won't impact the current workflow and can be replaced when we don't need it anymore, but will make transition from current acrg formats to any new formats easier.

Codecov Report

Merging #74 (ca1e84b) into devel (70ef136) will decrease coverage by 0.03%.
The diff coverage is 41.66%.

@@            Coverage Diff             @@
##            devel      #74      +/-   ##
==========================================
- Coverage   56.41%   56.37%   -0.04%
==========================================
  Files         106      106
  Lines        7062     7072      +10
==========================================
+ Hits         3984     3987       +3
- Misses       3078     3085       +7

Impacted Files
Coverage Δ

openghg/dataobjects/_obsdata.py
56.25% <41.66%> (-43.75%)
:arrow_down:

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 70ef136...ca1e84b. Read the comment docs.

Seems to be an issue with generating the documentation from the workflow notebook but I didn't make any changes on this branch to the notebook format.

Seems to be an issue with generating the documentation from the workflow notebook but I didn't make any changes on this branch to the notebook.
This is causing one of the test cases below to the fail (ubuntu 3.7) but all other versions seem to pass fine.

Yeah this is an issue that's been introduced with the new jinja release going 2 -> 3. In the searchRank branch I've updated the requirements to pin the jinja version until I get a chance to look into what's going on / something changes on their end.

Example code to check this:
from openghg.modules import ObsSurface
from openghg.localclient import get_obs_surface
import os
import tempfile
tmp_dir = tempfile.TemporaryDirectory()
os.environ["OPENGHG_PATH"] = tmp_dir.name # "/tmp/openghg_store"
home = os.getenv("HOME")
decc_file = os.path.join(home,"openghg/notebooks/tutorials/data/DECC/bsd.picarro.1minute.248m.dat")
decc_results = ObsSurface.read_file(filepath=decc_file, data_type="CRDS", site="bsd", network="DECC")
data = get_obs_surface(site="bsd", species="ch4", inlet="248m")
print("Type of data returned from get_obs_surface",type(data))
print(f"Extract Dataset {type(data.data)} using data.data")
print(data.data)
print(f"Can also extract Dataset {type(data['bsd'])} using legacy format data[sitecode]")
print(data["bsd"])
The ability to extract the Dataset using data["bsd"] is what has been added in this pull request.

Waiting on fix related to  jinja version going into devel, to check the tests all pass before this can be merged.
Would it possible to move this fix into devel (rather than the searchRank branch) so this can be checked?

source: gharchive/pull-request

authors: codecov-commenter gareth-j rt17603

repo: openghg/openghg


text:
Fix to stop CSV data being retrieved from ICSO CP

Summary of changes

This stops CSV data being retrieved from the ICOS Carbon Portal.

Please check if the PR fulfills these requirements

[x] Closes #864
[ ] Tests added and passed if fixing a bug or adding a new feature
[ ] All code checks passing - black --line-length 110 run over code, mypy and flake8 not showing any errors
[ ] Added an entry in the latest CHANGELOG.md file if fixing a bug or adding a new feature

Is the issue here just with the tests? If it's not too hard to fix it would be great if this PR could be included in the next release to save me switching to this branch all the time, although I appreciate that other things are higher priority.

I've also added an extra filter to stop project-specific drought files (e.g. https://meta.icos-cp.eu/objects/uWgpKU-q44efg-KX7_H3GHAY) from being retrieved. These contain old versions of data that is superseded by more recent European Obspack releases (e.g. https://meta.icos-cp.eu/objects/aJZSRhksf7GR3U3oZXoKRpgi) but for reasons that are unclear to me the drought files have not been marked as deprecated for all sites (although they have been in most cases).

authors: gareth-j joe-pitt


text:
Allow configurable assertion recipient field. Simplify assertion template creation

For deployment in multiple environments, there's a need to allow the recipient value in the Assertion to be configurable.
Simplify the assertion template creation to always create the template for every POST call to spcp. This would ensure that the 'issueInstant' field is always set with the latest date and time.

Given that you are an external contributor, would you be happy to fill up and sign the following CLA please? Thanks!

Hello @marvinkhristianzamora - have you had a chance to look at this?

Hi, I've signed the CLA and pushed latest updates based on your comments
Marvin Khristian Zamora
mklzamora@gmail.com
On Mon, Apr 8, 2019 at 8:27 AM LoneRifle notifications@github.com wrote:

Hello @marvinkhristianzamora https://github.com/marvinkhristianzamora -
have you had a chance to look at this?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/opengovsg/mockpass/pull/14#issuecomment-480646965,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABNl0RJBcM3VcIPjcT61g7XrNkb_9k9pks5veo0OgaJpZM4cRc31
.

@marvinkhristianzamora just deleting a comment so that your email address is not disclosed

authors: LoneRifle marvinkhristianzamora

repo: opengovsg/mockpass

license: MIT


text:
text generation webui not a model?

Comment on mastodon: https://amok.recoil.org/@nick/110813522014804587

That’s a great page for comparing degrees of openness. I just wonder why https://github.com/oobabooga/text-generation-webui is included, as it isn’t a model in and of itself, it’s a set of runtime tools with a web ui?

Correct, shouldn't be there, might be mentioned as a relevant resource somewhere


text:
chore: change copyright.

Due to TSC meeting at 2022-03-30,  https://github.com/openkraken/TSC/issues/38
Change the copyright to kraken authors.

@cnryb 刚才遗漏了一些, 这下都修改好了

repo: openkraken/kraken


text:
feat: add linux desktop support.

Add Linux platform support.
Fixed #853
TODO:

kraken_websocket

有代码冲突

authors: andycall yuanyan


text:
Nav appearance on "tablet" size and down

Best option is to use the hamburger nav at the tablet point.
It's hard to tell from the screenshot, but there's no padding to the right and the menu items quickly become squeezed right after this point.

Fixed by #58

repo: openlawteam/openlaw-website


text:
selected feature doesn't disappear

Hello
I have a problem with a selected feature. I can't remove it from the vector layer. It means the feature is removed from the layer, but still visible.

register select interaction on map
select feature
count number of features (ex. 32) - for test
remove feature from layer
count number of feature (ex. 31) - for test

So, the feature is removed from the layer, but still visible on the map. When I click someplace, the feature disappears.
With the following steps it works

register select interaction on map
select feature
unregister select interaction
remove feature from layer
register select interaction on map

it shows like a selected feature can't remove from layer resp. the map doesn't refresh the view.

This is because the feature is copied to the Select interaction's internal layer when it is selected, and it does not get notified when the feature is removed from its original layer.
I have experienced similar issues like this, all related to the fact that features are added to another layer by the Select interaction, and masked by the skipFeatureUid object on the map.
If we want to maintain this architecture, then a fix would be to register listeners for the original layer's remove event that removes features from the selection layer.
A related issue with the current approach is that features with the same id (feature.getId()) as an already selected feature from a different layer will disappear when selected, because a vector source can only have on feature with the same id.

authors: ahocevar rniederer


text:
OSM vector tiles example does not work

The OSM vector tiles example does not work.
http://openlayers.org/en/master/examples/osm-vector-tiles.html
The vector tiles are not loaded and the server responds with the message Mapzen has released Vector Tiles 1.0. For more information visit: https://mapzen.com/blog/v1-vector-tile-service/

Fixed with #5985?

Yes, didn't see that.

en/master/ has just been updated with latest master.

authors: marcjansen simonseyock


text:
Add getters to BingMaps source for api key and imagerySet

These options can be passed into the source constructor, but cannot be retrieved after construction. This is useful for serialising state.

thanks @fredj


text:
zIndex of unmanaged vector layer broken if zIndex=0 or unset

Describe the bug
The layer order of vector layer is broker on unmanaged layer if one layer has zIndex=0 or undefined. The order remains the order of insertion.
To Reproduce

Add two unmanged layer with zIndex zero -> the order of the layers is the order of insertion
change the zIndex of the first layer to 1 -> Layer should be on top of the second layer with index=0 but stays below
change first layer to zIndex=2 and second layer to index=1 -> layer order is correct

You can find the example here
Expected behavior
Layer order should be correct

interestingly zIndex="0" works

repo: openlayers/openlayers


text:
Cannot read property 'getStyleFunction' of null

I'm generating my own vector tiles in java. Everything works OK, until I zoom in a bit on the map. At some point I get this error:
DevTools failed to load source map: Could not load content for https://unpkg.com/elm-pep.js.map: HTTP error: status code 404, net::ERR_HTTP_RESPONSE_CODE_FAILURE
index.js:52 [WDS] Live Reloading enabled.
VectorTileLayer.js:260 Uncaught TypeError: Cannot read property 'getStyleFunction' of null
    at CanvasVectorTileLayerRenderer.render (VectorTileLayer.js:260)
    at _loop_1 (VectorTileLayer.js:278)
    at CanvasVectorTileLayerRenderer.updateExecutorGroup_ (VectorTileLayer.js:297)
    at CanvasVectorTileLayerRenderer.prepareTile (VectorTileLayer.js:131)
    at VectorRenderTile.Target.dispatchEvent (Target.js:116)
    at VectorRenderTile.Tile.changed (Tile.js:137)
    at VectorRenderTile.Tile.setState (Tile.js:238)
    at VectorTile.listenChange_1 (VectorTile.js:298)
    at VectorTile.Target.dispatchEvent (Target.js:116)
    at VectorTile.Tile.changed (Tile.js:137)
render @ VectorTileLayer.js:260
_loop_1 @ VectorTileLayer.js:278
CanvasVectorTileLayerRenderer.updateExecutorGroup_ @ VectorTileLayer.js:297
CanvasVectorTileLayerRenderer.prepareTile @ VectorTileLayer.js:131
Target.dispatchEvent @ Target.js:116
Tile.changed @ Tile.js:137
Tile.setState @ Tile.js:238
listenChange_1 @ VectorTile.js:298
Target.dispatchEvent @ Target.js:116
Tile.changed @ Tile.js:137
Tile.setState @ Tile.js:238
VectorTile.setFeatures @ VectorTile.js:126
VectorTile.onLoad @ VectorTile.js:110
xhr.onload @ featureloader.js:86
load (async)
loadFeaturesXhr @ featureloader.js:67
(anonymous) @ VectorTile.js:446
VectorTile.load @ VectorTile.js:100
(anonymous) @ VectorTile.js:313
TileGrid.forEachTileCoord @ TileGrid.js:185
VectorTile.getSourceTiles @ VectorTile.js:270
VectorRenderTile.load @ VectorRenderTile.js:147
TileQueue.loadMoreTiles @ TileQueue.js:120
PluggableMap.handlePostRender @ PluggableMap.js:914
(anonymous) @ PluggableMap.js:1197
setTimeout (async)
PluggableMap.renderFrame_ @ PluggableMap.js:1195
(anonymous) @ PluggableMap.js:197
requestAnimationFrame (async)
PluggableMap.render @ PluggableMap.js:1090
PluggableMap.handleViewPropertyChanged_ @ PluggableMap.js:1016
Target.dispatchEvent @ Target.js:116
BaseObject.notify @ Object.js:182
BaseObject.set @ Object.js:214
View.applyTargetState_ @ View.js:1467
View.updateAnimations_ @ View.js:702
requestAnimationFrame (async)
View.updateAnimations_ @ View.js:736
requestAnimationFrame (async)
View.updateAnimations_ @ View.js:736
requestAnimationFrame (async)
View.updateAnimations_ @ View.js:736
requestAnimationFrame (async)
View.updateAnimations_ @ View.js:736
requestAnimationFrame (async)
View.updateAnimations_ @ View.js:736
requestAnimationFrame (async)
View.updateAnimations_ @ View.js:736
requestAnimationFrame (async)
View.updateAnimations_ @ View.js:736
requestAnimationFrame (async)
View.updateAnimations_ @ View.js:736
requestAnimationFrame (async)
View.updateAnimations_ @ View.js:736
requestAnimationFrame (async)
View.updateAnimations_ @ View.js:736
View.animateInternal @ View.js:607
View.animate @ View.js:529
Zoom.zoomByDelta_ @ Zoom.js:129
Zoom.handleClick_ @ Zoom.js:108

OK, Seems when generating features, I have "UNKNOWN" instead of some geometry string... So it is my code in java and not JS...


text:
How to get latitude and longitude and visual area when the map is repeated and slides in one direction

How to get latitude and longitude and visual area when the map is repeated and slides in one direction

Hi
Can you be a little more explicit to have a chance to get an answer and some help ?

toLonLat will normalize wrapped coordinates https://codesandbox.io/s/geographic-forked-vswpi

for info #13030 ...

Same. Use ol/extent#getBottomLeft() and ol/extent#getTopRight(), and ol/proj#toLonLat() on the results.

Thank you

When I render the point to the map, the map is looping, but the point does not


text:
Unforgiving get flat expression

Describe the bug
When I use "text-value": ["get", "ECO_NAME"] flat style for the ecoregions.json, I get Expected a string for text-value. This is because of 821 features, there is one that has an undefined ECO_NAME.
To Reproduce
Steps to reproduce the behavior:

Go to https://codesandbox.io/p/sandbox/vector-layer-forked-6833gw?file=%2Fmain.js%3A23%2C20

Snippet
import GeoJSON from 'ol/format/GeoJSON.js';
import Map from 'ol/Map.js';
import VectorLayer from 'ol/layer/Vector.js';
import VectorSource from 'ol/source/Vector.js';
import View from 'ol/View.js';

const vectorLayer = new VectorLayer({
  background: '#1a2b39',
  source: new VectorSource({
    url: 'https://openlayers.org/data/vector/ecoregions.json',
    format: new GeoJSON(),
  }),
  style: {
    'fill-color': ['string', ['get', 'COLOR'], '#eee'],
    'text-value': ["get", "ECO_NAME"]
  },
});

const map = new Map({
  layers: [vectorLayer],
  target: 'map',
  view: new View({
    center: [0, 0],
    zoom: 1,
  }),
});

const featureOverlay = new VectorLayer({
  source: new VectorSource(),
  map: map,
  style: {
    'stroke-color': 'rgba(255, 255, 255, 0.7)',
    'stroke-width': 2,
  },
});

let highlight;
const displayFeatureInfo = function (pixel) {
  const feature = map.forEachFeatureAtPixel(pixel, function (feature) {
    return feature;
  });

  const info = document.getElementById('info');
  if (feature) {
    info.innerHTML = feature.get('ECO_NAME') || ' ';
  } else {
    info.innerHTML = ' ';
  }

  if (feature !== highlight) {
    if (highlight) {
      featureOverlay.getSource().removeFeature(highlight);
    }
    if (feature) {
      featureOverlay.getSource().addFeature(feature);
    }
    highlight = feature;
  }
};

map.on('pointermove', function (evt) {
  if (evt.dragging) {
    return;
  }
  const pixel = map.getEventPixel(evt.originalEvent);
  displayFeatureInfo(pixel);
});

map.on('click', function (evt) {
  displayFeatureInfo(evt.pixel);
});

Expected behavior
The expression should not simply fail. I can think of three solutions in my order of preference:

provide a fallback for get -> workaround is to use ["string", ["get", "ECO_NAME"], "fallback"]
add a coalesce expression -> ["string", ["get", "ECO_NAME"], "fallback"] seems to provide this functionality
silently ignore the missing ECO_NAME and display nothing

PS.: The case operator is unsupported for text-value

OL version 8.2.0

Try this:
            'text-value': [
              'match',
              ['!', ['get', 'ECO_NAME']],
              true,
              '',
              ['get', 'ECO_NAME'],
            ],

There are sufficient workarounds. That's why I'm closing this for now.

authors: StefanBrand ahocevar


text:
Feature request: adjust imagery offset

It would be nice if u can add a feature to adjust imagery offset. Or is there any such feature in openlayers?

You'd have to be more specific. What is your use case? What imagery? What offset?

Imagery offset means the adjustment needed to bring to satellite imagery layer (by panning it) to correct the position of the satellite imagery which reflects the actual GPS position of an object appears in the imagery.` For example: There are a difference of approximately 10 feet in the position of an object in google imagery & bing imagery. So one layer needs to be permanently moved (panned) to match another imagery layer by using an offset. You can also see this feature in openstreet maps editor. Because Bing imagery is little far away from actual GPS position of an object appears.

http://viglino.github.io/ol3-ext/examples/map.geoimage.html

That's for static images. What i'm looking for is, imagery offset for tiled layers

like this demo?
https://codepen.io/anon/pen/YZQzGE?editors=1010

Thank you! It solved my problem.

Caveat: the codepen above uses internal non-API properties. To achieve such offsets, it is better to create an ol.tilegrid.TileGrid with an appropriate origin from scratch. But in general, the approach to set an offset through a modified tile grid origin is a viable one.

authors: ChouGary ahocevar dhivehi


text:
Feature requests: Immediately render the linedash of ol.style.Stroke in WebGL mode

Hello guys,
The line-dash feature for ol.style.Stroke works very well in Canvas-rendering mode.
Cloud you please make the line-dash feature for ol.style.Stroke in WebGL mode available as well?

I have discovered that the ol.style.Stroke configuration parameters "strokeStyleLineCap", "strokeStyleLineDash", "strokeStyleLineDashOffset" and "strokeStyleLineJoin" are ignored in the JS file "ol-v4.3.1/ol/ol/render/webgl/linestringreplay.js".
Why?

These are not implemented for the WebGL renderer.

Will this option be implemented into the WebGL Renderer in the future?

In v6, there is a new WebGL renderer for points only.

Will it support lineDash on a LineString Stroke as well?

authors: DwieDave ahocevar stuartZhang


text:
Fix feature lookup after removal of alpha

This is another Layer#getFeatures(pixel) fix: After the removal of alpha (#10228), we cannot use the rgb value (0, 0, 0) for encoding features any more, because that is equal to having no feature at that pixel.

هذا Layer#getFeatures(pixel)حل آخر : بعد إزالة alpha ( # 10228 ) ، لا يمكننا استخدام قيمة rgb (0 ، 0 ، 0) لترميز الميزات بعد الآن ، لأن ذلك يساوي عدم وجود ميزة في هذا البكسل.


text:
Make Overlay.panIntoView an API method

Add parameters for custom animation parameters and margin

Add protected wrapper method for previous autoPan functionality

See #10741

It could be nice to have the function take a single options object.  This would have animation and margin properties and would allow more options to be added in the future without an awkwardly long signature.
To do this, you would create a new type as is done with the existing constructor options.

I agree. It would also then make sense to alter the autoPan options to use the new type rather than being two separate parameters to the constructor (and two internal properties).

so autoPanOptions in the constructor options instead of autoPanAnimation and autoPanMargin, and then carry the change through the whole class? autoPanAnimation and autoPanMargin could be left in as deprecated options for backward compatibility to avoid breaking the API.

@tschaub the test failures (rendering test) appear to be unrelated to this change: Could you force the build to run again?

@ahocevar Nice and obvious idea - should have thought of that myself!

authors: ejn tschaub


text:
Fix (MS)Pointer-based touch events for IE11+

This PR contains three commits, each dealing with a sub-problem of the current handling of (MS)Pointer-based touch events:

one test in Events fails as in browsers with pointer events we listen to an additional 4 pointer events

changes in tests/Events.html

IE11 drops the MS prefixes for pointer events: MSPointerDown -> pointerdown, etc. Furthermore the pointerType property of pointer event objects is no longer a numeric constant on the event object (e.g. e.MSPOINTER_TYPE_MOUSE) but now a string (e.g. "touch"). Additionally, not all devices set e.buttons to 0 for pointermove events (Surface 1 with WinRT 8.1 seems to, Lumia WP8.1 not)

changes in lib/OpenLayers/Events.js

handling of multitouch events in the drag handler was a bit wonky, as in the pointer model multiple events are fired rather than a single event. An attempt was being made to polyfill this by adding a touches array to the event, but the very first event didn't have this as there is no way of knowing that this will be part of a multitouch event. This led to e.g. pinch-zooming being a little dicey as some bits of the multitouch were being interpreted differently and the map tended to spring a bit. By tracking the pointerId of the events in the drag handler we can reliably tell whether it is really a single-finger drag or a multitouch gesture.

changes in lib/OpenLayers/Handler/Drag.js

Although these are three separate fixes for three distinct problems, they are all parts of the general problem that pointer event handling (particularly pinch zoom) is unsatisfactory in IE11 on touch devices, hence I've rolled them all into one PR. If preferred I can split into 3 separate PRs.

@ahocevar Thanks for looking at this - of course I meant _TOUCH_MODEL! I've updated the PR, and also taken the opportunity to remove some whitespace noise in the original.

Thanks @ejn for this impressive contribution!

authors: ahocevar ejn


text:
Replace STOP RUN with GOBACK

Signed-off-by: Hartanto Ario Widjaya tanto259@users.noreply.github.com
One of the issue with having STOP RUN is that it causes problems with subroutine CALL. In particular if a user decide to call a subroutine twice, and the subroutine contain a STOP RUN, the entire thing will terminate before the second execution.
In particular, this may have causes this issue on Slack: https://openmainframeproject.slack.com/archives/C011NE32Z1T/p1611697726073500
Thus, I believe it's in the best interest to replace all occurrence of STOP RUN to GOBACK since the programs will terminate more gracefully (i.e. the CALL-ed program will return control back to the caller instead of terminating everything).

@paulnewt any thoughts on this? If accepted, we will need to update the COBOL programs on the system.

@paulnewt any thoughts on this? If accepted, we will need to update the COBOL programs on the system.

STOP RUN replaced with GOBACK in all source located in ZOS.PUBLIC.SRC, ZOS.PUBLIC.ADHOC,CBL, and ZOS.PUBLIC.DB2.CBL

STOP RUN replaced with GOBACK in all source located in ZOS.PUBLIC.SRC, ZOS.PUBLIC.ADHOC,CBL, and ZOS.PUBLIC.DB2.CBL

authors: MikeBauerCA paulnewt tanto259


text:
[MLS Spec change] Define key schedule and reference in Figure 1 (#564)

Link to the exact changes
https://github.com/mlswg/mls-protocol/pull/564
Description of the changes

Note that ## Secret Tree is currently under # Key Schedule, so that should probably move or the top level section should be renamed. I ran out of imagination for today.

Editorial.

repo: openmls/openmls


text:
Update NANO33 PID/VID

#define USBD_PID                     (0x015A)
#define USBD_VID                     (0x2341)

Done, but, you need to update the firmware build files in board config... they have the regular openmv cam IDs right now.

authors: iabdalkader kwagyeman

repo: openmv/openmv-ide


text:
Vision, first prototype (reborn)

The first vision prototype, as it works it can safely go into master. Further developments will be added later as a separate pull request.

Merged as it was already merged by Tony before.

repo: opennars/opennars


text:
doc: Update docker-compose.yaml example

Fixes #

Proposed Changes

Release Note

Thanks for the contribution,great work

authors: klaxonz qingchoulove

repo: opennaslab/kubespider


text:
[pred_mod] Update test_resources path

Describe your changes

Update the test_resources paths
Create new test_resources
Upload test resources to AWS

Issue ticket number and link
Closes #xxxx (Replace xxxx with the GitHub issue number)
Checklist before requesting a review

[x] I have performed a self-review of my code

Check the correct box. Does this PR contain:

[ ] Breaking changes
[ ] New functionality
[ ] Major changes
[x] Minor changes
[ ] Bug fixes

[x] Proposed changes are described in the CHANGELOG.md

[x] CI Tests succeed and look good!

Requirements after merging

[ ] Need to regenerate common/ resources

[x] Need to regenerate task-specific resources. Specify: <all or ...>

viash ns test -q predict_modality succeeds locally as well!

authors: KaiWaldrant rcannood


text:
Add APIResourceListing class

This change is

Coverage decreased (-3.0%) to 45.899% when pulling 5713abbcdccb7dd31bd56f8a91aa4f604d5efcc2 on annawzz:a329191765491209_move_get_listing into d90f7d6950630cda031c51fdab35501677562856 on openprocurement:a258001251842882_refactoring.

Coverage decreased (-3.0%) to 45.899% when pulling 5713abbcdccb7dd31bd56f8a91aa4f604d5efcc2 on annawzz:a329191765491209_move_get_listing into d90f7d6950630cda031c51fdab35501677562856 on openprocurement:a258001251842882_refactoring.

authors: annawzz coveralls


text:
ARR: Add missing Venue functions

This PR adds functions that are present in the Venue class but not in the ARR class

can you add a test that recruits reviewers using the request form to make sure everything is working?

authors: haroldrubio melisabok

repo: openreview/openreview-py


text:
AssertionError in org.openrewrite.java.tree.JavaType$Method#getDeclaringType()

When running rewriteRun in Gradle with rewrite-spring:4.15.0 the following exception is thrown:
java.lang.AssertionError
	at org.openrewrite.java.tree.JavaType$Method.getDeclaringType(JavaType.java:879)
	at org.openrewrite.java.MethodMatcher.matches(MethodMatcher.java:116)
	at org.openrewrite.java.search.UsesMethod.visitJavaSourceFile(UsesMethod.java:46)
	at org.openrewrite.java.search.UsesMethod.visitJavaSourceFile(UsesMethod.java:24)
	at org.openrewrite.java.tree.J$CompilationUnit.acceptJava(J.java:1198)
	at org.openrewrite.java.tree.J.accept(J.java:51)
	at org.openrewrite.TreeVisitor.visit(TreeVisitor.java:198)
	at org.openrewrite.RecipeScheduler.lambda$scheduleVisit$4(RecipeScheduler.java:187)
	at org.openrewrite.RecipeScheduler.lambda$mapAsync$0(RecipeScheduler.java:50)
	at org.openrewrite.scheduling.ForkJoinScheduler.lambda$schedule$0(ForkJoinScheduler.java:41)

This is the relevant code:
https://github.com/openrewrite/rewrite/blob/v7.17.2/rewrite-java/src/main/java/org/openrewrite/java/tree/JavaType.java#L879
First observation: The assertion checks that the declaringType field is not null. But the corresponding constructor parameter is annotated with @Nullable.
Is this a bug or could there maybe be a problem with how the tooling is used?

This is a bug. It's only temporarily @Nullable while the instance is being built up during parsing, but should be non-null by the time it's accessed.

@michel-zimmer while we are big fans of lombok and use it ourselves, the abstract syntax tree OpenRewrite produces isn't aware of annotation processors, and any methods/constructors/etc they may produce.
We plan to support lombok and similar annotation processors, but this work has not yet begun.
Essentially what we have to do is take the AST from the Java compiler running without lombok, then use the type information from the Java compiler with lombok.

I see, no problem. I can imagine, that it's far from trivial to support annotation processors, that modify the AST without a standardized mechanism how AST manipulation works.
We will continue to use the delombok function as we are in the lucky position that Lombok is primarily used in our production code and we are using the JUnit 4 -> 5 rewrite which affects our test code as one would expect.

Hi @michel-zimmer. I've removed the assertion. Maybe we'll put it back in once we can better support lombok. Utilities that work with types, like MethodMatcher, wont be able to do anything with the null declaring type, but they wont explode with an NPE. Your example shows that removing the assertion is less disruptive than keeping it.

authors: jkschneider michel-zimmer sambsnyd

repo: openrewrite/rewrite


text:
ChangePropertyKey not respecting sequentiality

Hi folks! I don't understand sequentiality in some OpenRewrite recipes. I reproduce this simple example:
public Sequential() {
    this.doNext(new ChangePropertyKey("common.tracing", "common.execution.context", true, null, null));
    this.doNext(new DeleteProperty("common.sleuth.send", null, null, null));
    this.doNext(new ChangePropertyKey("common.sleuth", "common.tracing", true, null, null));
  }

And this test should pass, but it fails because where I expect common.tracing appears as common.execution.context. If the changes are applied sequentially, it has no sense!
void changeOnlyDesiredProperties() {
    this.rewriteRun(yaml("""
        common:
          cache:
            enabled: true
          sleuth:
            send: true
            sender:
              http:
                username: user
                password: pwd
                connectionRequestTimeout: 5000
                timeout: 3000
                readTimeout: 3000
          blockhound.enabled: true
        """,
        """
            common:
              cache:
                enabled: true
              tracing:
                sender:
                  http:
                    username: user
                    password: pwd
                    connectionRequestTimeout: 5000
                    timeout: 3000
                    readTimeout: 3000
              blockhound.enabled: true
                    """));
  }

Slack Message

We had a similar fix just last week for the Maven recipe RenamePropertyKey;
Perhaps a similar approach would work here for this Yaml recipe.
Would you be interested in contributing a fix to OpenRewrite?

Hi guys@timtebeek @JoseLora , would you mind let me take on this issue? I am willing to spend some time to investigate this one.

We had a similar fix just last week for the Maven recipe RenamePropertyKey; Perhaps a similar approach would work here for this Yaml recipe.
Would you be interested in contributing a fix to OpenRewrite?

Hi @timtebeek, It looks like there isn't same functional method(maybeUpdateModel()) in YamlVisitor.class. I am thinking of having to implemet one same functionality method for YamlVisitor.  Just want to check with you whether my understanding is correct?

Hi @RossZhang-cpu ; I think that maybeUpdateModel is pretty unique to our MavenVisitor, and only needed there to update a marker containing information derived from the Maven pom.xml file contents. For Yaml our model is simpler, and does not have that concept of derived information markers that I know of, so no need to write an equivalent maybeUpdateModel.
In this case I suspect it's mostly an issue with the recipes themselves, but perhaps we can start with a draft PR containing just the above test, and explore what's needed with a debugger from there on out.

Thank you @timtebeek for classification. I raised a draft PR only containing test, please kindly help to check.

Hi @timtebeek, after debugging I found the defaut value of cycle is 2, which result into another Recipe Execution and change the property key again. Please see the snapshots below:

Here maxCycles is 2 will cause another unexpected Recipe execution.

It seems can't customize cycles vale from the begining in current codes. Do you have any clue about it?

Yes, the test framework always runs 2 cycles. The reason is that any recipe can request that another cycle should be run, so the test framework always runs 2 cycles to make sure that the recipe behaves correctly in this scenario.
Here I believe the recipe is actually behaving as expected as the third recipe produces a new common.tracing property which in the second cycle will be transformed by the first recipe.
To avoid this the simplest solution is to set maxCycles: 1` on the YAML recipe, which I believe would be adequate in this use case.
So in summary I believe there is nothing to be fixed for this particular issue. By creating a test case and stepping through with the debugger and finding what caused this behavior I think you learned a lot about OpenRewrite and hope you don't feel your time was completely wasted and can instead apply that knowledge to look into another issue that is bothering you.
Unless you or @timtebeek see something else that needs fixing here, I suggest that we close the issue and the PR again.

Thank you @knutwannheden  for sharing information.
Even debugger takes me sometime, I am still feel happy to learn something new during that time.
And I have tried to set cycle to be 1 in test code, it works fine.
rewriteRun(
                spec -> {
                    spec.recipeFromYaml(yamlRecipe, "org.openrewrite.ChangePropertyKeySequentialityTest");
                    spec.cycles(1);
                    }

But may I know how to implement you mentioned before set on YAML recipe
To avoid this the simplest solution is to set maxCycles: 1` on the YAML recipe

Since Recipe declares a maxCycles() method I was thinking we probably allow declarative YAML recipes to specify a corresponding value. But it turns out we don't. This could be an interesting addition for use cases as in this issue. Let's see what @timtebeek thinks about this.

So Knut's proposal is up for discussion in:

https://github.com/openrewrite/rewrite/pull/3850

With that I think we've all learned something here and I think this issue can be closed. Thanks for taking a look @RossZhang-cpu ! Always good seeing how you dive into issues here.

authors: JoseLora RossZhang-cpu knutwannheden timtebeek


text:
setTracking(true) not working immediately for mousetracker

I implemented some mousetrackers that do something when I move the mouse.
With setTracking(true/false), I activate (or deactivate) them (e.g. with a shortcut on the keyboard).
Deactivating works immediately, as expected.
But Activating does not: I call setTracking(true), then I have to either leave the canvas and re-enter it first, or click a mouse button, and then the tracker is actually active.
Is there a function that I can call to "register" the active mousetracke explicitly?

Strange! I don't see anything in the code that would account for that:
https://github.com/openseadragon/openseadragon/blob/6abcc76a93ee3369267908f0a80641dc171440ed/src/mousetracker.js#L1460
What events are you tracking? Sounds like mouseMove? And you toggle it with a keyboard shortcut? I've never heard of this problem, so I'm wondering if there's something specific about your scenario.
@msalsbery Any thoughts?

I think the only problem that could occur her is that re-enabling the tracker also resets the tracker state. The code directly calls addEventListener so the event should be fired - that's not a problem. I think inside the event handler it decides to ignore the first event that comes because of the tracker state reset, that might be the reason why it works after you trigger the state change (canvas focus/click/etc)...

Thanks for the suggestions.
Well, when I activate the mousetracker with .setTracking(true), I get a console warning:
mousetracker.js:2985 updatePointerCaptured() called on untracked pointer
Maybe this has to do with it. Still investigating...

I remember encountering this issue once too. The problem is IMHO that unless that some event happens first the state of the re-enabled tracker is unknown, similarly to implementing using custom html element for the mouse cursor and setting the html element its initial position before any event was fired; and you don't know what's the cursor position (browsers do not keep this value anywhere unless you save it manually). Maybe if the tracker also registered for mouse move event, then the tracker revival would happen sooner. But then you have touch screens etc...

Thanks for the suggestions.
Well, when I activate the mousetracker with .setTracking(true), I get a console warning:
mousetracker.js:2985 updatePointerCaptured() called on untracked pointer
Maybe this has to do with it. Still investigating...

That sounds like you're activating and/or deactivating during a pointer button down state, which will be problematic.
If that's the case, a better approach would be to leave the tracker active but filter event handling based on whatever state logic you need.
This codepen demonstrates some of the ways you can intercept MouseTracker events

Thanks, @msalsbery , this worked for me as a workaround.
Still, I do not activate the tracker during a pointer button down state: I have the same behavior as described in the beginning when I activate the tracker from Chrome's console: the mousetracker will still only start to work when I do an action.
However, your workaround works: I leave the mousetracker active from the beginning, but handle the logic with a flag which I set true when I want the tracker to be effective.
I think we can close this, since there is a workaround. However, the root cause is not clear to me. Note that deactivating the tracker with setTracking(false) always had immediate effect.

@jetic83 What elements are you attaching these MouseTrackers to?

@msalsbery  on the navigator, via it's id.

@msalsbery  on the navigator, via it's id.

Cool thank you!
Is that the id of an element you're hosting the navigator in?
(Just trying to debug in a similar scenario)

@msalsbery , well in OSD you can set the id of the navigator. And in the mousetracker, you can also set the id of the element. That is the same ID.

@msalsbery , well in OSD you can set the id of the navigator. And in the mousetracker, you can also set the id of the element. That is the same ID.

@jetic83  Great thank you, I'll keep you posted
Edit: The workaround is a good solution still...I do want to fix reactivating issues though

authors: Aiosa iangilman jetic83 msalsbery

repo: openseadragon/openseadragon


text:
Viewer updateSize

I have a scenario where I create when the element width is 1920 ,but display is none. I found the result of 'viewer.viewport. getContainerSize' is {x:1,y:1}, when I was to make the display element for block, I try to use the viewer. ForceResize to update, it won't work,But I observed the result of 'viewer.viewport. getContainerSize' is {x:1920,y:1080},Why doesn't he work? Maybe I want a similar one viewer.navigator.updateSize.I found it to have the effect I wanted

You need to give the viewer a defined height too via css.


text:
Auto restore RED indices

Is your feature request related to a problem? Please describe.
RED Cluster is one of the major issue in today’s elasticsearch and opensearch cluster, where because of various reasons indices turn RED and hence searches will return partial results, and indexing into that index will return errors.
In simple words cluster is RED means: some primary shards are not assigned, which means that cluster data is not fully available in cluster.

Describe the solution you'd like:
To this problem I am proposing solution of auto recovering these RED indices from past successful snapshots with no data loss.
Recovering RED indices is two step process:

Identifying there will be no data loss after recover.
Recover RED indices from past successful snapshots.

Identifying there will be no data loss after recover:
For each shard opensearch maintains two special seq#:

local checkpoint# - this is the highest seq# for which all lower seq# have been processed in data node .
global checkpoint# (or just checkpoint#) - the highest seq# for which the local shard can guarantee that all previous (included) seq# have been processed on all active shard copies (i.e., primary and replicas).This Global check runs at every 30s interval.

Each and every opensearch node maintain sequence number of segments for the shard they are holding. When ever there is any update operation to a shard this sequence number is incremented. These two numbers are maintained in memory but also persisted in the metadata of every lucene commit.
public class SeqNoStats implements ToXContentFragment, Writeable {
private static final String SEQ_NO = "seq_no";
private static final String MAX_SEQ_NO = "max_seq_no";
private static final String LOCAL_CHECKPOINT = "local_checkpoint";
private static final String GLOBAL_CHECKPOINT = "global_checkpoint";

private final long maxSeqNo;
private final long localCheckpoint;
private final long globalCheckpoint;

But problem with sequence number is that it is maintained at each node level and if the node goes down, we lose the sequence number information details.
Hence Idea is to maintain the index write status at cluster level. Below is the approach via which we can determine if an index is fully recoverable from past snapshots without data loss or not:

Index Write :

Whenever there is index write to an index in Data Node

Check if snapshot is in progress
If Snapshot for index is in progress

If clean_to_restore_from_snapshot  is TRUE

Update clean_to_restore_from_snapshot to FALSE  in index setting
Update Index_Update_Time_Stamp with current time stamp  in index setting

Else If clean_to_restore_from_snapshot  is FALSE and Index_Update_Time_Stamp  is less than snapshot_start_time, then

Update Index_Update_Time_Stamp with current time stamp  in index setting

Else

NO ACTION

Else

if clean_to_restore_from_snapshot for index is  TRUE

Update clean_to_restore_from_snapshot to FALSE in index setting
Update Index_Update_Time_Stamp with current time stamp  in index setting

Else

NO ACTION

Snapshot :

When Snapshot creation completes for an index

If snapshot for the index was sucessful

If clean_to_restore_from_snapshot is FALSE

If Index_Update_Time_Stamp > SNAPSHOT_START_TIME for the index,

DO NOTHING , as there was index write after snapshot was started for the index.

Else update clean_to_restore_from_snapshot to TRUE

Else if Snapshot was failure for the index

NO ACTION

Batching Updates:
In order to ensure we don’t update cluster state for same index again and again for same value, batching mechanism is been  introduced. Batching mechanism works in following way:

Data nodes and Master node Push all request to update index setting to queue in elected master node
Elected master flush all request from queue every 30 seconds
Batch all request request together for same index together
Update index setting clean_to_restore_from_snapshot and Index_Update_Time_Stamp as needed.

Setting to be introduced as part of this feature:

clean_to_restore_from_snapshot : Index setting (Boolean) indicate if there was any write to index or not after past successful snapshot for the index.
clean_to_restore_from_snapshot_update_time : Index setting (TimeSatmp) indicate time when “clean_to_restore_from_snapshot” time was updated . [restore _snapshot_update_time?]
auto_restore_red_indices_enable : Cluster Setting (Boolean) to enable or disable auto recover red indices feature. TRUE by default.
auto_restore_red_indices_if_no_data_loss : Cluster Setting (Boolean) to choose if to restore RED indices if there was data ingested to index after past successful snapshot. FALSE by default.
auto_restore_red_indices_repository : Cluster Setting (String) to choose from which repository to auto restore red indices.

Pre-Conditions for auto recover:

RED Indices are HOT indices
There is no change in data after past successful snapshot in the RED indices (Configurable: This condition can be relaxed with cluster setting)
There is enough disk available on cluster to restore the data
JVM and CPU utilization is below 80% (configurable)
RED Indices are present in past snapshots
Recover Failure throttling limit is not reached

Recovering RED Indices:
We will have a scheduler running on elected master at at regular interval of time to recover RED indices .The recovery will work in following fashion:

Check existing RED index in regular interval(customer customizable, defaults to 60 mins)
and make sure the index is in RED state for x amount of time (customer customizable, defaults 24*60 mins) ,
and the index is/are recoverable from past snapshots with no data loss.
No snapshot restore in-progress
No snapshot deletion is in-progress
Get latest Snapshot Index mapping in which index is present
For all indices for which the above conditions are met it will

Close index
Get latest Snapshot in which the index is present from Snapshot Index mapping
recover the index and
reset the  “clean_to_restore_from_snapshot’ to true

Speedup Recover process:
Snapshot recovery is time consuming process and can take hours for index to get restored. Our idea is to optimize the total time taken to restore index. We will implement following mechanism to speedup restore process:

Restore at shard level
Increase upper limit of number of SNAPSHOT threads used for RESTORE operation

Api to track auto restore:
Will introduce an API to to track past auto restore events that happened . with response containing following details :

List of red Indices auto recovered with SUCCESS/FAILURE
Total time taken to restore (with start time and end time)
Snapshot name and snapshot time from which indices are been recovered

Additional context
Add any other context or screenshots about the feature request here.

Hi @piyushdaftary!
I really like this design, at Logz.io we have a (very) similar backend functionality already in place. We have experience implementing such mechanisms in production using mostly external services.
Therefore, I have one major suggestion -
Please split the feature into two parts:

the decision of whether a cluster requires a recovery operation AND is ready for that operation
the recovery operation task and logic

I highly recommend starting with (2) and creating endpoints to trigger the action first.
The reason is that (1) may differ based on the way you manage the cluster! maybe it would be better as a plugin of its own.
For example - if the node count is not what we expect it to be (cluster can be red due to data node failure) then we won't want to perform the recovery yet. A k8s operator may have trouble allocating the required resources for the new data node, and in the meantime, the recovery will start, not knowing the situation outside the cluster. There are different approaches to this process so I think it is more suitable as part of the mechanism of a k8s operator or other cluster governance operators out there. It seems like the "interchangeable" portion of the feature.
Having said that, I think if you consider that there is always data that has been indexed after the last snapshot you can minimize the size of the feature:

Anyone using OpenSearch for logging probably does periodically create snapshots but they are momentary and logs are always flowing in. Statistically speaking, logs always arrive after the last snapshot.
Recovery is better than not recovering, if you implement just the recovery process in one phase and the "auto" part as a plugin then users can decide how and when to do this.

I am having a call this week with the people that implemented this level of code at logz.io. I will add more insight if there is any once I have done that.

Hi @AmiStrn
Thanks for looking into the proposal and liking the design :) .
If I break the feature into two parts, then just implementing "recovery operation" first wouldn't add much value to opensearch as there already exists an API to restore indices from past snapshots.
As opensearch is self sufficient software, I feel it's better to implement the default version of restore feature (As proposed in design) as default in core and expose interface. So that if anyone want to implement different logic of restoration can easily implement it via their plugin. The default version of restore feature will ensure that restore is only triggered if there are enough resources available on cluster. Please see section "Pre-Conditions for auto recover:" section in design for details.
If customer wants to turn off the auto restore feature they can easily do it dynamically by updating cluster setting auto_restore_red_indices_enable to false.
For snapshot time, Yes you are right, its already available in repository and we are not again storing snapshot time. The two different time metrics are been captured in the feature design are :

When an index became dirty (means there was difference in data of index in cluster w.r.t. data in repository) and index setting clean_to_restore_from_snapshot updated
Restoration time of RED index by autorecover feature.

The Design also ensures that we don't update index setting clean_to_restore_from_snapshot on every index write/update. It is updated only when there is first write to index or if there is any write to index after last successful snapshot of the index. Details of the logic is mentioned in "Index Write :" and "Snapshot :" section of design. This also ensures we don't update the index setting unnecessarily for Inactive indices.
Customer can also update the setting auto_restore_red_indices_if_no_data_loss to TRUE if they want to restore only red indices where restoration will lead to no data loss. For remaining red indices they can plan to restore as they want.
Please share your view and also please feel free to point out if I missed anything.

If I break the feature into two parts, then just implementing "recovery operation" first wouldn't add much value to opensearch as there already exists an API to restore indices from past snapshots.

I think this separation will have some benefits

the PRs will be smaller
the separation is a logical one allowing other code to use this recovery based on different conditions
the separation allows for easy test separation as well

By saying separation I'm not saying you should abandon the auto recover part, just separate it in terms of phases and logic.
However, after running massive clusters for years I can say for sure that the most common reason for a red cluster is node failure. This means that in the design under "Recovering RED Indices:"  (I assume you are missing the validation section there) the part where you validate the cluster is ready for the recovery will most likely (from experience) fail until the cluster is ready. which means there will be many failed attempts until the cluster is ready, which may take a while, resulting in many redundant checks rather than only beginning the process when the cluster is ready for it.
At logz.io we use an event-based approach:
If the cluster is red (and a snapshot exists) the recovery starts by fixing the cluster nodes (if required), and then the event of that being done triggers the recovery from the snapshots.
This approach requires monitoring of the cluster health externally since we need to fix the nodes.
If it was a must that the cluster do this on its own, then the validation should have some sort of backoff mechanism to revalidate if the cluster is ready after some time. Otherwise, it will be a busy-wait or failed recovery situation pretty much every time a cluster is red due to node failure (which is almost every time a cluster is red).
Sometimes users will use shard allocation and routing settings to allocate certain shards to certain nodes. In this case, simply having enough disk space and CPU/JVM won't be enough to start the recovery. I feel like this part really needs to be handled more carefully. A default auto-recovery should be really conservative in my opinion if it is to be implemented.
If you want to go through with the full design you propose anyway, then please at least separate it as you said, so there are:

pluggable way to override and change the auto-recovery behavior.
the recovery part is separate allowing other users to use it in a flexible way (via API perhaps?)

Cheers:)
Amitai

Thanks @AmiStrn for your inputs.
I will try to phase out the PRs so that it is easy to review.
As Opensearch is self sufficient software, hence want the autorecover logic to be in core software logic itself.
Will make recover interface abstract so that if someone want to write custom plugin for recover they can easily do that.
I have added precondition check (please refer to section : "Pre-Conditions for auto recover"), to ensure existing cluster is capable of restoring from past snapshots. Please Let me know if I should add more conditions here.
Please do feel free to revert incase I missed anything.

What about my comment regarding: https://www.elastic.co/guide/en/elasticsearch/reference/master/modules-cluster.html#modules-cluster
There may be other cases like this..
I suggest looking at the implications and adding to the section you referenced regarding pre-conditions.
I asked other people maintaining production clusters and the general response I get is that the nodes must be fixed first, about 95% of the time, before recovery is done, and wherever there are routing settings things get even more complex. Please verify with opensearch operations teams at AWS that this design fits their needs, I am curious what they think as well:)
Also, I am not familiar with the notion of this system being "self sufficient".
I mean, the cluster can't reach out and repair its own hardware (yet!) - where do we draw the line on self sufficiency? In this case the logic would be to fix nodes and then recover, but in this design the cluster can only just try to recover in a rare set of cases that meet of all the preconditions (and flags). If you were the devOps person, would you truly rely on this working most of the time?
Thanks for the response!
Amitai

Hi @AmiStrn
Ack on point to explicitly do precondition check for shard allocation and routing settings before triggering  recover(missed to call it out in proposal). Thanks for suggesting it.
By self sufficient I mean say that instead of any external process triggering recovery,  opensearch code itself should be able to do the recovery.
It's true opensearch can't repair hardware on its own. To fix a hardware you need manual intervention which is usually time consuming process and during this duration data availability will be impacted. In multiple data node cluster, let's say if few of nodes failed which makes cluster to go in red state and remaining of the available cluster can sustain the load of cluster then autorestore will come as savior and will help in data availability in cluster. We have seen many instances (much more than 5% for sure) where autorecover can help without even manual intervention. And once let say after manual intervention the node failure is been fixed and node joins back the cluster, then shard relocation via BalancedShardsAllocator will evenly distribute the shards across data nodes. The main focus of auto restore feature is to make data available for search as soon as possible.

remaining of the available cluster can sustain the load of cluster

This implies the cluster could have been smaller in the first place. Which is usually not the case when trying to reduce costs.
Its almost as if you propose to have more memory and nodes than required in order for this to fully be operational?

authors: AmiStrn piyushdaftary

repo: opensearch-project/OpenSearch


text:
[RFC] Modeling writes as an extensible workflow

Note 1 - The discussion in this issue focuses on the write paths in OpenSearch, though the assertions herein are probably true for other parts of the codebase.

Note 2 - For clarity, the term “replication” will only be used to describe code paths that result in segment files being created on a shard. The act of sending requests from the primary shard to replicas will be termed “forwarding”.

What's the problem?
Much of the code in the write path for segments is tightly coupled together. For example:

The implementation of index and delete operations uses a class hierarchy that mandates that these operations be replicated.
The notion of “replication” is coupled to “forwarding” (see Note 2 above)
The notion of a “write” is tightly coupled with a translog update.

(I'll talk about why we would want to solve for these in a moment.)
This coupling is driven by the code architecture. In technical terms, I'd say it uses a compile-time, top-down (inheritance-based) chain of responsibility (CoR) design pattern. Put more simply, it's like spaghetti lasagna - lots of layers encasing lots of noodley code.
Such a design pattern poses two problems:

The compile-time nature precludes the ability to configure behavior at run-time
The inheritance-based CoR pattern implicitly defines a fixed set of steps for the code, but misses out on the benefits of a unified orchestrator class or workflow definition - for example, the ability for a step to react to the result of a previous step

I think we can make this better (though that recipe is beyond saving IMO).
What should we do about it?
We should rearchitect write-path operations as a workflow comprised of the following configurable steps:

Reroute (route the incoming request to the correct shard/node)
Ingest (process the request)
Persist (make the results of the request durable)

This would include separate configuration/extension points for storage and translog

Forward (send the request to another node)

Persist and Forward will be conditional steps that rely on the output of prior steps to determine if they should execute.
Why do this now?
Because extensibility is one of key themes for Opensearch (https://github.com/opensearch-project/OpenSearch/issues/2095). It is essential that we start tackling this architectural limitation now since we have multiple ongoing initiatives for OpenSearch extensibility that require more run-time configurability:

With the introduction of replication strategies like segment replication being defined per-index, write code paths can no longer simply mandate replication. Segment replication no longer needs “replication” to be coupled with “forwarding”.
With a remote translog, the need for “forwarding” is removed entirely.
The introduction of remote storage will affect the behavior of both replication and recovery.

Open Questions (aka things I'm mulling over)

What situations/architectures (if any) would require the Reroute step to be optional/configurable?
Is there a way to remove the need for an Engine class, so that ingest and translog can be configured independent of one another?
How does this workflow and the decoupling of replication vs forwarding affect sync actions?

Given the sheer breadth of functionality in the Opensearch codebase, there are probably other coupled components that I haven't considered. Please comment below if there are things that would break with this workflow approach, or other areas that may benefit from a similar approach.

Few observations

With https://github.com/opensearch-project/OpenSearch/issues/1319, the need for “forwarding” is removed entirely.

There are few cases today that inherently rely on forwarding writes for correctness, for instance if the primary is partitioned off from the rest of the cluster, it continues to accept writes, once it forwards the request to the replica which meanwhile has been promoted to the primary, it then becomes aware of the problem and hence respond with a failure which otherwise would have been acknowledged and writes diverged.
Forwarding is a nice property to detect such anomalies. We need to give it more thought before we get rid of this

Is there a way to remove the need for an Engine class, so that ingest and translog can be configured independent of one another?

With https://github.com/opensearch-project/OpenSearch/issues/1319, translogs would be decoupled from the Engine and would be made optional and extracted out
Would that simplify the Engine?

At a high level this makes a lot of sense to me. If you see a refactor increment that would make the code better I would PR that on main. An alternative could be to try and setup the new workflow as proposed without touching the existing implementation, that could be a good start in a feature branch.

There are few cases today that inherently rely on forwarding writes for correctness, for instance if the primary is partitioned off from the rest of the cluster, it continues to accept writes, once it forwards the request to the replica which meanwhile has been promoted to the primary, it then becomes aware of the problem and hence respond with a failure which otherwise would have been acknowledged and writes diverged.

Thanks @Bukhtawar ! These are exactly the kind of blind spots I was hoping to get feedback on. I'd like to understand this particular behavior more in depth. Are you aware of any reading material to learn more about this? Or could you point me to other resources (code, exceptions, etc.) from where I could start learning?

With https://github.com/opensearch-project/OpenSearch/issues/1319, translogs would be decoupled from the Engine and would be made optional and extracted out
Would that simplify the Engine?

Decoupling translog behavior from Engine would indeed simplify its code. Assuming the next step is to decouple ingest/storage from Engine, I'm left wondering what value the Engine class itself is adding by encapsulating those two extension points.

@nknize I saw this PR comment from you so I'd love to hear what you think about the ideas here, especially around what value the Engine class provides (as discussed above 👆 )

authors: Bukhtawar dblock kartg


text:
[Concurrent Segment Search] Add support for query profiler with concurrent aggregation

Placeholder issue to support query profiler with concurrent aggregation flow (both for global and non-global aggregation). This will be a subtask for #7354

Background
The search profile query section includes detailed timing information about the execution of the query tree by Lucene on a particular shard. The breakdown component further provides detailed statistics on the low-level execution performed by Lucene. Existing types of breakdown for query profile timing/count include:

create_weight: This property measures the elapsed time, in nanoseconds, for generating a Weight object, which serves as a temporary context object to store state associated with the (IndexSearcher, Query) tuple.
build_scorer: This property measures the elapsed time, in nanoseconds, for constructing a Scorer object for the query. It records the time taken to generate the Scorer, rather than the actual scoring of documents.
next_doc: This property measures the elapsed time, in nanoseconds, for determining the next matching document, which can vary significantly depending on the complexity of the query.
advance: This property measures the elapsed time, in nanoseconds, for finding the next matching doc in the lower level of the next_doc property.
match: This property measures the elapsed time, in nanoseconds, for matching documents in two-phase process.
score: This property measures the elapsed time, in nanoseconds, for scoring a specific document using its Scorer.
shallow_advance: This property measures the elapsed time, in nanoseconds, for executing the advanceShallow Lucene method.
compute_max_score: This property measures the elapsed time, in nanoseconds, for executing the getMaxScore Lucene method.
set_min_competitive_score: This property measures the elapsed time, in nanoseconds, for executing the setMinCompetitiveScore Lucene method.

Currently, with the model of concurrent search, the query portion of the profile response in the  ConcurrentQueryProfileBreakdown class, introduced in #1673, displays the cumulative summary timing of the query tree.
@Override
public Map<String, Long> toBreakdownMap() {
    final Map<String, Long> map = new HashMap<>(buildBreakdownMap(this));

    for (final AbstractProfileBreakdown<QueryTimingType> context : contexts.values()) {
        for (final Map.Entry<String, Long> entry : buildBreakdownMap(context).entrySet()) {
            map.merge(entry.getKey(), entry.getValue(), Long::sum);
        }
    }

    return map;
}

The contexts map has all the timing information across all segments. The key of the contexts map is LeafReaderContext, which is created per segment. For example, in the 2 slices case, if we have 4 segments, then we still get 4 context key-value pairs instead of 2 pairs. As a result, the total timing obtained by summing all segments does not represent the slice-level detailed timing of the query during concurrent execution.
Proposed Solution
In the concurrent search case:

Introduce profiled time stats (i.e. min/max/avg) for all existing slice-level breakdown properties.
create_weight and time_in_nanos field do not have profiled time stats because they are executed at the query-level rather than the slice-level.
The existing slice-level breakdown time properties will reflect the total elapsed time across all segment slices (i.e. max(slice_end_time) - min(slice_start_time))
Introduce profiled count stats (i.e. min/max/avg) for all existing slice-level breakdown properties.
Maintain the same profile as today in the non-concurrent search case. For example, since there is no specific timer for time_in_nanos field, its timing is instead reflected as the cumulative summary of all timing types.

Sample query response
Non-concurrent search
{
    ...
    "profile":
    {
        "shards":
        [
            {
                "id": "[aggqMTQ4QbOShmjsbcXtrQ][index][0]",
                "inbound_network_time_in_millis": 0,
                "outbound_network_time_in_millis": 0,
                "searches":
                [
                    {
                        "query":
                        [
                            {
                                "type": "MatchAllDocsQuery",
                                "description": "*:*",
                                "time_in_nanos": 382251,
                                "breakdown":
                                {
                                    "set_min_competitive_score_count": 0,
                                    "match_count": 0,
                                    "shallow_advance_count": 0,
                                    "set_min_competitive_score": 0,
                                    "next_doc": 6708,
                                    "match": 0,
                                    "next_doc_count": 11,
                                    "score_count": 7,
                                    "compute_max_score_count": 0,
                                    "compute_max_score": 0,
                                    "advance": 1875,
                                    "advance_count": 3,
                                    "score": 2959,
                                    "build_scorer_count": 6,
                                    "create_weight": 102875,
                                    "shallow_advance": 0,
                                    "create_weight_count": 1,
                                    "build_scorer": 267834
                                }
                            }
                        ],
                        "rewrite_time": 8501,
                        "collector": [...]
                    }
                ],
                "aggregations": [...]
            }
        ]
    }
}

Concurrent search with 3 segment slices
{
    ...
    "profile":
    {
        "shards":
        [
            {
                "id": "[09K0cflDSwO6kVx95Gj-eA][index][0]",
                "inbound_network_time_in_millis": 0,
                "outbound_network_time_in_millis": 0,
                "searches":
                [
                    {
                        "query":
                        [
                            {
                                "type": "MatchAllDocsQuery",
                                "description": "*:*",
                                "time_in_nanos": 271500,
                                "breakdown":
                                {
                                    "create_weight": 171556,
                                    "create_weight_count": 1,
                                    "build_scorer": 1691375,
                                    "max_build_scorer": 576136,
                                    "min_build_scorer": 551446,
                                    "avg_build_scorer": 563791,
                                    "build_scorer_count": 6,
                                    "max_build_scorer_count": 2,
                                    "min_build_scorer_count": 2,
                                    "avg_build_scorer_count": 2,
                                    "next_doc": 3834,
                                    "max_next_doc": 1334,
                                    "min_next_doc": 1222,
                                    "avg_next_doc": 1278,
                                    "next_doc_count": 8,
                                    "max_next_doc_count": 3,
                                    "min_next_doc_count": 2,
                                    "avg_next_doc_count": 3,
                                    ...
                                }
                            }
                        ],
                        "rewrite_time": 10083,
                        "collector": [...]
                    }
                ],
                "aggregations": [...]
            }
        ]
    }
}

Alternatives
1. Expand query profile stats across all slices with concurrent execution
Instead of exposing min/max/avg stats across segment slices execution of a collector manager, we list out all the collectors response from the collector manager.
Sample query response for a concurrent search with 3 segment slices
{
    ...
    "profile":
    {
        "shards":
        [
            {
                "id": "[09K0cflDSwO6kVx95Gj-eA][index][0]",
                "inbound_network_time_in_millis": 0,
                "outbound_network_time_in_millis": 0,
                "searches":
                [
                    {
                        "query":
                        [
                            {
                                "type": "MatchAllDocsQuery",
                                "description": "*:*",
                                "time_in_nanos": 90500,
                                "breakdown":
                                {
                                    "set_min_competitive_score_count": 0,
                                    "match_count": 0,
                                    "shallow_advance_count": 0,
                                    "set_min_competitive_score": 0,
                                    "next_doc": 3834,
                                    "match": 0,
                                    "next_doc_count": 8,
                                    "score_count": 8,
                                    "compute_max_score_count": 0,
                                    "compute_max_score": 0,
                                    "advance": 2958,
                                    "advance_count": 3,
                                    "score": 5459,
                                    "build_scorer_count": 6,
                                    "create_weight": 271500,
                                    "shallow_advance": 0,
                                    "create_weight_count": 1,
                                    "build_scorer": 1691375
                                }
                            },
                            {
                                "type": "MatchAllDocsQuery",
                                "description": "*:*",
                                "time_in_nanos": 90500,
                                "breakdown":
                                {
                                    "set_min_competitive_score_count": 0,
                                    "match_count": 0,
                                    "shallow_advance_count": 0,
                                    "set_min_competitive_score": 0,
                                    "next_doc": 2334,
                                    "match": 0,
                                    "next_doc_count": 8,
                                    "score_count": 5,
                                    "compute_max_score_count": 0,
                                    "compute_max_score": 0,
                                    "advance": 1587,
                                    "advance_count": 3,
                                    "score": 5125,
                                    "build_scorer_count": 2,
                                    "create_weight": 271500,
                                    "shallow_advance": 0,
                                    "create_weight_count": 1,
                                    "build_scorer": 1691234
                                }
                            },
                            {...}
                        ],
                        "rewrite_time": 10083,
                        "collector": [...]
                    }
                ],
                "aggregations": [...]
            }
        ]
    }
}

2. Keep the current cumulative summary version of the timing breakdown
Currently, no code changes are required for this approach. However, it is important to note that we will lose the slice-level profile details.
Sample query response for a concurrent search with 3 segment slices
{
    ...
    "profile":
    {
        "shards":
        [
            {
                "id": "[OHv1ByGtTHqVf55eR4Bvsg][index][0]",
                "inbound_network_time_in_millis": 0,
                "outbound_network_time_in_millis": 0,
                "searches":
                [
                    {
                        "query":
                        [
                            {
                                "type": "MatchAllDocsQuery",
                                "description": "*:*",
                                "time_in_nanos": 129584,
                                "breakdown":
                                {
                                    "set_min_competitive_score_count": 0,
                                    "match_count": 0,
                                    "shallow_advance_count": 0,
                                    "set_min_competitive_score": 0,
                                    "next_doc": 15418,
                                    "match": 0,
                                    "next_doc_count": 26,
                                    "score_count": 16,
                                    "compute_max_score_count": 0,
                                    "compute_max_score": 0,
                                    "advance": 3042,
                                    "advance_count": 3,
                                    "score": 7876,
                                    "build_scorer_count": 6,
                                    "create_weight": 129584,
                                    "shallow_advance": 0,
                                    "create_weight_count": 1,
                                    "build_scorer": 765164
                                }
                            }
                        ],
                        "rewrite_time": 11333,
                        "collector": [...]
                    }
                ],
                "aggregations": [...]
            }
        ]
    }
}

Tagging @sohami @yigithub @jed326 @andrross @reta for feedback. Thank you.

@ticheng-aws thanks for the proposal

As a result, the total timing obtained by summing all segments does not represent the slice-level detailed timing of the query during concurrent execution.

This is correct, the timing is cumulative. When I originally worked on the feature, the ideal picture for the profiling I had in mind was "perceived time", not avg / min / max. What I mean by that: we know that some slices will be processed concurrently, some will be queued (and hopefully processed concurrently later on). By "perceived time" I envisioned the time between first chunk of slices being picked up and the last chunk of slices being completed. That the time that consumers will see as the query execution. The proposal by itself adds useful information, I think what is missed is the fact there is concurrency involved (so the numbers don't  end up, they rather interleave, in general case).

authors: reta ticheng-aws


text:
[Meta][Segment Replication]Tests failing due to segment replication specific behavior

Using this issue to track tests that are failing when segment replication is enabled. These tests only include failures caused by differing behavior of segment replication from document replication.
For each test, we need to decide if -

it will be excluded, or
we create a segment replication specific test for it, or
we need a fix to change the segment replication behavior

[ ] NoClusterManagerNodeIT.testNoClusterManagerActionsWriteClusterManagerBlock #9837


text:
[Version] Don't spoof major for 3.0+ clusters

Changes version comparison logic to only translate major version when comparing with legacy 7x versions.
This is needed beginning in 2.0 so that when running 2.0+ versions in bwc mode for 3.0+ upgrades, node
versions no  longer have to translate major version or spoof to legacy versions.

Logic for supporting legacy versions remain; for now. Will remove in a follow up PR

#1703 repeat... refiring

start gradle check

Hi, I found a test failure that likely caused by this commit (found in a gradle check run https://github.com/opensearch-project/OpenSearch/pull/2752#issuecomment-1088013983). I believe you are working on a fix 😁
REPRODUCE WITH: ./gradlew ':server:test' --tests "org.opensearch.cluster.coordination.JoinTaskExecutorTests.testPreventJoinClusterWithUnsupportedNodeVersions" -Dtests.seed=445AF7CCE2D6222A -Dtests.security.manager=true -Dtests.jvm.argline="-XX:TieredStopAtLevel=1 -XX:ReservedCodeCacheSize=64m" -Dtests.locale=vi-VN -Dtests.timezone=Asia/Vladivostok -Druntime.java=17

org.opensearch.cluster.coordination.JoinTaskExecutorTests > testPreventJoinClusterWithUnsupportedNodeVersions FAILED
    junit.framework.AssertionFailedError: Expected exception IllegalStateException but no exception was thrown
        at __randomizedtesting.SeedInfo.seed([445AF7CCE2D6222A:C72550B63015B266]:0)
        at org.apache.lucene.tests.util.LuceneTestCase.expectThrows(LuceneTestCase.java:2898)
        at org.apache.lucene.tests.util.LuceneTestCase.expectThrows(LuceneTestCase.java:2884)
        at org.opensearch.cluster.coordination.JoinTaskExecutorTests.testPreventJoinClusterWithUnsupportedNodeVersions(JoinTaskExecutorTests.java:122)

This may have caused https://github.com/opensearch-project/OpenSearch/issues/3615, care to help @nknize?

This may have caused https://github.com/opensearch-project/OpenSearch/issues/3615, care to help @nknize?

Ack, will take a look.

authors: dblock nknize tlfeng


text:
Add a link to the flakey test dev guide.

Signed-off-by: Daniel (dB.) Doubrovkine dblock@amazon.com
Description
Added some text to help the contributor with next steps on CI failures.
Issues Resolved
Closes https://github.com/opensearch-project/OpenSearch/issues/4871.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Codecov Report

Merging #4872 (b418ec0) into main (3af46ae) will decrease coverage by 0.05%.
The diff coverage is 62.50%.

:exclamation: Current head b418ec0 differs from pull request most recent head 844722d. Consider uploading reports for the commit 844722d to get more accurate results

@@             Coverage Diff              @@
##               main    #4872      +/-   ##
============================================
- Coverage     70.70%   70.65%   -0.06%
+ Complexity    57896    57388     -508
============================================
  Files          4689     4576     -113
  Lines        277137   274896    -2241
  Branches      40313    40288      -25
============================================
- Hits         195947   194223    -1724
+ Misses        64987    64456     -531
- Partials      16203    16217      +14

Impacted Files
Coverage Δ

...main/java/org/opensearch/plugins/SearchPlugin.java
76.92% <ø> (ø)

.../main/java/org/opensearch/search/SearchModule.java
96.24% <ø> (-0.51%)
:arrow_down:

...earch/search/aggregations/InternalAggregation.java
82.71% <ø> (+4.93%)
:arrow_up:

...arch/search/aggregations/InternalAggregations.java
76.47% <0.00%> (+1.96%)
:arrow_up:

...egations/pipeline/AvgBucketPipelineAggregator.java
73.33% <ø> (ø)

...ions/pipeline/BucketMetricsPipelineAggregator.java
75.00% <ø> (+14.28%)
:arrow_up:

...tions/pipeline/BucketScriptPipelineAggregator.java
60.78% <ø> (ø)

...ons/pipeline/BucketSelectorPipelineAggregator.java
0.00% <ø> (ø)

...gations/pipeline/BucketSortPipelineAggregator.java
0.00% <ø> (ø)

...ions/pipeline/CumulativeSumPipelineAggregator.java
73.07% <ø> (ø)

... and 835 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

org.opensearch.repositories.s3.RepositoryS3ClientYamlTestSuiteIT > test {yaml=repository_s3/20_repository_permanent_credentials/Snapshot and Restore with repository-s3 using permanent credentials} FAILED
    java.lang.AssertionError: Failure at [repository_s3/20_repository_permanent_credentials:152]: expected [2xx] status code but api [snapshot.create] returned [400 Bad Request] [{"error":{"root_cause":[{"type":"invalid_snapshot_name_exception","reason":"[repository_permanent:snapshot-one] Invalid snapshot name [snapshot-one], snapshot with the same name already exists","stack_trace":"InvalidSnapshotNameException[[rep

https://github.com/opensearch-project/OpenSearch/issues/2064

Again
 - org.opensearch.repositories.s3.RepositoryS3ClientYamlTestSuiteIT.test {yaml=repository_s3/20_repository_permanent_credentials/Try to create repository with broken endpoint override and named client}

@dblock please add this to 1.x and 1.3 as well.
Thanks.

@dblock please add this to 1.x and 1.3 as well.
Thanks.

Will do. I think I want to edit this a bit on main first.

authors: codecov-commenter dblock peterzhuamazon


text:
[Refactor] more ImmutableOpenMap to jdk Map in cluster package

This continues the refactor of ImmutableOpenMap to j.u.Map in the o.o.cluster package in preparation for removal of HPPC dependencies.
relates #5910

Surprised a bunch of downstreams: https://github.com/opensearch-project/security/issues/2714.

@nknize @dblock  The 2.x backport workflow failed here.

authors: dblock khushbr nknize


text:
Exclude InnocuousThread from thread access permissions check

Description
Excluded InnocuousThread from thread access permissions check, since
InnocuousThread - threads which have no permissions, are not members of any user-defined ThreadGroup and supports the ability to erase ThreadLocals.
This fix will help to solve the issue with bumping up of OpenSAML to the latest version in the security plugin.
Related Issues
No issue
Check List

[ ] New functionality includes testing.

[ ] All tests pass

[ ] New functionality has been documented.

[ ] New functionality has javadoc added

[x] Commits are signed per the DCO using --signoff
[x] Commit changes are listed out in CHANGELOG.md file (See: Changelog)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@willyborankin we should not go with this change. The InnocuousThread are purposely there since they are created from the thread pools that are not managed by OpenSearch. Opening them up bypasses all security checks in place and opens up a whole in the system. Instead, we should find out where those threads are coming from, which thread pool, and supply the one managed by OpenSearch.

@reta move it to Draft so fat

authors: reta willyborankin


text:
[BUG][security.yml] deploy custom security config is not supported if auth type is different than oidc

If copy_custom_security_configs is true custom file security_plugin_conf.yml is not deployed because the check is made on  auth_type == 'oidc'.
- name: Security Plugin configuration | Copy the security configuration file 3 to cluster
  template:
    src: security_plugin_conf.yml
    dest: "{{ os_sec_plugin_conf_path }}/config.yml"
    backup: yes
    owner: "{{ os_user }}"
    group: "{{ os_user }}"
    mode: 0600
    force: yes
  when: auth_type == 'oidc'

So if you customised security_plugin_conf.yml to use ldap or anything but oidc then you cannot deploy the configuration.
I suggest an easy fix by adding an alternative check :
- name: Security Plugin configuration | Copy the security configuration file 3 to cluster
  template:
    src: security_plugin_conf.yml
    dest: "{{ os_sec_plugin_conf_path }}/config.yml"
    backup: yes
    owner: "{{ os_user }}"
    group: "{{ os_user }}"
    mode: 0600
    force: yes
  when: auth_type == 'oidc' or copy_custom_security_configs

This changed should not cause too much trouble.

[Triage]
Hey @french-kiwy thanks for raising an issue, I now get the problem, can you please raise a PR and contribute to the fix you suggested?
Thank you
@gaiksaya @bbarani @peterzhuamazon

authors: french-kiwy prudhvigodithi

repo: opensearch-project/ansible-playbook


text:
adding release notes for 2.3.0

Signed-off-by: Bharathwaj G bharath78910@gmail.com
Description
Adding release notes for 2.3.0
Issues Resolved
https://github.com/opensearch-project/asynchronous-search/issues/173
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Codecov Report

:exclamation: No coverage uploaded for pull request base (main@ed1962d). Click here to learn what that means.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #183   +/-   ##
=======================================
  Coverage        ?   83.46%
  Complexity      ?      556
=======================================
  Files           ?       61
  Lines           ?     2050
  Branches        ?      146
=======================================
  Hits            ?     1711
  Misses          ?      234
  Partials        ?      105

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

authors: bharath-techie codecov-commenter

repo: opensearch-project/asynchronous-search


text:
[BUG] Use snapshots from distribution/maven for dependencies

Describe the bug
There are -SNAPSHOT builds checked into https://github.com/opensearch-project/cross-cluster-replication/tree/main/src/test/resources/security/plugin. This is error prone at the very least, use a distribution build.

There are -SNAPSHOT builds checked into https://github.com/opensearch-project/cross-cluster-replication/tree/main/src/test/resources/security/plugin. This is error prone at the very least, use a distribution build.

we have addressed this in this PR: https://github.com/opensearch-project/cross-cluster-replication/pull/296 but keeping this issue open until the distribution builds are published to maven to use in CI workflow

why was this closed? it's not yet using opensearch-security from maven:
https://github.com/opensearch-project/cross-cluster-replication/blob/a3d0bf0308a556764e428fe6c1bd3539bd3eb190/.github/workflows/security-tests.yml#L26-L36
=> this ticket should IMHO be re-opened as it isn't resolved.

Hey @ankitkala @rursprung just to connect the dots once the issue is resolved https://github.com/opensearch-project/security/issues/1801, the required zip files can be fetched from maven, if then can this issue be resolved?
@dblock @bbarani
Example (once the zips are published to maven) as
mvn org.apache.maven.plugins:maven-dependency-plugin:2.1:get  -DrepoUrl=https://aws.oss.sonatype.org/content/repositories/snapshots/  -Dartifact=org.opensearch.plugin:opensearch-security:1.3.0.0-SNAPSHOT:zip

you shouldn't have to manually run mvn in the GitHub Action - instead you should probably have the dependency in your build.gradle so that it also works in any other place where you build the project. otherwise you wouldn't really have to use maven but could just download the ZIP from any download location.

you shouldn't have to manually run mvn in the GitHub Action - instead you should probably have the dependency in your build.gradle so that it also works in any other place where you build the project. otherwise you wouldn't really have to use maven but could just download the ZIP from any download location.

Hey @rursprung, I have just added as an example, but yes its should be fetched as dependency in gradle.

Hey Starting with release 2.1.0, we now have plugin zips in maven repo
We should be good to remove the checked in zips from repo from 2.x branches.
https://github.com/opensearch-project/opensearch-build/issues/1916#issuecomment-1179276885
SecurityZip maven: https://repo1.maven.org/maven2/org/opensearch/plugin/opensearch-security/
@saikaranam-amazon @dblock @rursprung @ankitkala
Thank you

@prudhvigodithi: see my comment here: https://github.com/opensearch-project/index-management/pull/403#issuecomment-1179663724
i think what's still missing to resolve this is the publishing of the ZIP files to the snapshot maven repo during snapshot builds. unless i was looking in the wrong place...? but then the solution for index-management is IMHO still wrong.

Hey @rursprung, official release zips are part of maven repo, snapshots can be fetched from nexus maven repo
Example for [job-scheduler snapshots] (https://aws.oss.sonatype.org/content/repositories/snapshots/org/opensearch/plugin/opensearch-job-scheduler/)

Just to add the PluginZips are out in maven starting 2.1.0, 1.x versions have to be backfilled.
Thank you
@rursprung @dblock @bbarani

Just to add the PluginZips are out in maven starting 2.1.0, 1.x versions have to be backfilled.
Thank you
@rursprung @dblock @bbarani

@saikaranam-amazon @ankitkala whats the next step here?

We'll  take this up in upcoming 1-2 weeks. @bbarani Are there any timelines you're looking at?

Closing the issue now.

authors: ankitkala bbarani dblock prudhvigodithi rursprung saikaranam-amazon


text:
Kafka Plugin: support for SASL/SCRAM mechanisms

Description
Add support for SASL/SCRAM mechanisms
example config:
authentication:
  sasl:
    scram:
      username: user1234
      password: password1234
      mechanism: SCRAM-SHA-512

Issues Resolved
Resolves #4241
Check List

[x] New functionality includes testing.
[ ] New functionality has a documentation issue. Please link to it in this PR.

[ ] New functionality has javadoc added

[x] Commits are signed with a real name per the DCO

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@chenqi0805 I did the rebase. Happy to help.

Hi @chenqi0805 although it would be a good idea to implement the credential refreshment right away I might have to pass on this one. I tried to comprehend the architecture behind it and could not really grasp it. Thus I have no understanding on where to put what to make this a clean and complete implementation. So I would prefer to keep this out of this particular PR.

Hi @chenqi0805 although it would be a good idea to implement the credential refreshment right away I might have to pass on this one. I tried to comprehend the architecture behind it and could not really grasp it. Thus I have no understanding on where to put what to make this a clean and complete implementation. So I would prefer to keep this out of this particular PR.

@franky-m No problem! I can create an issue for that and take over.

Thank you @franky-m for this contribution!

Our migration from ElasticSearch to OpenSearch is currently blocked by this topic. Is there yet a schedule for the next release that will include this?

authors: FLimburg chenqi0805 dlvenable franky-m

repo: opensearch-project/data-prepper